D12511: optimization of KTextEditor::DocumentPrivate::views()

[Jaime Torres Amate]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit R39:90157a222b5e: optimization of 
KTextEditor::DocumentPrivate::views() (authored by jtamate).

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12511?vs=33613&id=33644

REVISION DETAIL
  https://phabricator.kde.org/D12511

AFFECTED FILES
  src/document/katedocument.cpp
  src/document/katedocument.h

To: jtamate, #kate, cullmann, mwolff
Cc: mwolff, cullmann, #frameworks, michaelh, kevinapavew, ngraham, bruns, 
demsking, sars, dhaumann

D12511: optimization of KTextEditor::DocumentPrivate::views()

[Milian Wolff]

mwolff accepted this revision.
mwolff added a comment.


  OK, cool! That clearly shows that this patch _is_ valuable: Before we have 
~6% CPU cycle cost, now it's down to 1.5% (inclusively). This is a significant 
reduction, so I'm all for it.
  
  But note once again the stark difference in these numbers vs. what valgrind 
reports. Please, try to use `perf record --call-graph dwarf` + hotspot 
flamegraphs more in favor of callgrind/kcachegrind. Note that you can even use 
hotspot's record page for some more features, most notably the off-CPU 
profiling. This can give you more food for potential optimizations (finding 
sleep time, I/O, lock contention, ...).
  
  Happy profiling!

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12511

To: jtamate, #kate, cullmann, mwolff
Cc: mwolff, cullmann, #frameworks, michaelh, kevinapavew, ngraham, bruns, 
demsking, sars, dhaumann

D12511: optimization of KTextEditor::DocumentPrivate::views()

[Jaime Torres Amate]

jtamate added a comment.


  In D12511#258212 , @mwolff wrote:
  
  > 
https://phabricator.kde.org/file/data/w4qogv4brtxlc5p5bnwr/PHID-FILE-q62giymcptudpl5m6bt3/kwrite_perf_after_25_dwarf_caller.png
 shows ~1.5% in notifyAboutRangeChange (inclusively). Is that before or after 
your patch here?
  
  
  After applying the patch.
  Before applying the patch: F5833542: kwrite_perf_before_25_dwarf_caller.png 


REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12511

To: jtamate, #kate, cullmann, mwolff
Cc: mwolff, cullmann, #frameworks, michaelh, kevinapavew, ngraham, bruns, 
demsking, sars, dhaumann

D12511: optimization of KTextEditor::DocumentPrivate::views()

[Milian Wolff]

mwolff added a comment.


  `perf record -g` produces unusable data files, since it relies on the frame 
pointer which is usually not available. Use `perf record --call-graph dwarf` 
instead. 
https://phabricator.kde.org/file/data/w4qogv4brtxlc5p5bnwr/PHID-FILE-q62giymcptudpl5m6bt3/kwrite_perf_after_25_dwarf_caller.png
 shows ~1.5% in notifyAboutRangeChange (inclusively). Is that before or after 
your patch here?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12511

To: jtamate, #kate, cullmann, mwolff
Cc: mwolff, cullmann, #frameworks, michaelh, kevinapavew, ngraham, bruns, 
demsking, sars, dhaumann

D12511: optimization of KTextEditor::DocumentPrivate::views()

[Jaime Torres Amate]

jtamate added a comment.


  In D12511#258193 , @mwolff wrote:
  
  > Actually, no. Ignore what I said. The pictures you are showing are pretty 
meaningless. Did you run perf with `--call-graph dwarf`? Better look at the 
flamegraph and search for the function you are interested in 
(Kate::TextBuffer::notifyAboutRangeChange) or use the Caller/Callee view to get 
an aggregated view of your change.
  
  
  That's not the impression I got running kwrite in live and under a slow 
machine (valgrind), before I had to wait for the paste to finish, and after the 
patch not. But kwrite is still slow undoing the paste.
  
  I did run perf record -g, the same test with the patch applied after pasting 
the text 25 times and undoing with this result: F5833430: 
kwrite_perf_after_25.png .
  The same test (pasting 25 times and undoing) with --call-graph dwarf with 
this result: F5833432: kwrite_perf_after_25_dwarf.png 
, and F5833445: 
kwrite_perf_after_25_dwarf_caller.png .

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12511

To: jtamate, #kate, cullmann, mwolff
Cc: mwolff, cullmann, #frameworks, michaelh, kevinapavew, ngraham, bruns, 
demsking, sars, dhaumann

D12511: optimization of KTextEditor::DocumentPrivate::views()

[Milian Wolff]

mwolff added a comment.


  Actually, no. Ignore what I said. The pictures you are showing are pretty 
meaningless. Did you run perf with `--call-graph dwarf`? Better look at the 
flamegraph and search for the function you are interested in 
(Kate::TextBuffer::notifyAboutRangeChange) or use the Caller/Callee view to get 
an aggregated view of your change.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12511

To: jtamate, #kate, cullmann, mwolff
Cc: mwolff, cullmann, #frameworks, michaelh, kevinapavew, ngraham, bruns, 
demsking, sars, dhaumann

D12511: optimization of KTextEditor::DocumentPrivate::views()

[Milian Wolff]

mwolff added a comment.


  But the hotspot screenshot clearly shows that you are spending time on 
optimizing things that are barely noticeable. You have optimized a function 
that consumes 0.3% of the CPU cycles. It now consumes only ~0.15%, at the cost 
of slightly higher memory consumption.
  
  This is the real cost. The instruction numbers reported by callgrind paint a 
very different picture (22.6% vs. 5.5%). But that's not what actually matters, 
the cycle cost is way more important.
  
  Also note that hotspot shows inclusive costs. You should get used to it, most 
notably have a look at the flamegraph and get acquainted with that way to 
visualize profiling costs.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12511

To: jtamate, #kate, cullmann, mwolff
Cc: mwolff, cullmann, #frameworks, michaelh, kevinapavew, ngraham, bruns, 
demsking, sars, dhaumann

D12511: optimization of KTextEditor::DocumentPrivate::views()

[Jaime Torres Amate]

jtamate updated this revision to Diff 33613.
jtamate edited the test plan for this revision.
jtamate added a comment.


  Add and remove from the list.
  
  About the tools I use to profile:
  
  - I'm used to use slow machines, I have no problems running under valgrind.
  - When I can run a program and I'm able to repeat the workload that produces 
high cpu usage or slowness, I do like to use callgrind/kcachegrind, just 
because I get inclusive costs.
  - When a program gets slow after some time running or under some unexpected 
circumstances, yes I use perf, because I do not need to restart the program and 
I can do live measurements.
  - But I **do love** the kcachegrind view of the % of instructions for every 
method within the source code.
  
  In any case, the hotspot results pasting 10 times and undoing:
  Before: F5833105: kwrite_perf_before.png 

  After: F5833106: kwrite_perf_after.png 

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12511?vs=33574&id=33613

REVISION DETAIL
  https://phabricator.kde.org/D12511

AFFECTED FILES
  src/document/katedocument.cpp
  src/document/katedocument.h

To: jtamate, #kate, cullmann, mwolff
Cc: mwolff, cullmann, #frameworks, michaelh, kevinapavew, ngraham, bruns, 
demsking, sars, dhaumann

D12511: optimization of KTextEditor::DocumentPrivate::views()

[Milian Wolff]

mwolff added a comment.


  Oh and again: please start using perf/hotspot instead of callgrind. Really, 
the performance numbers you get from callgrind are just *instructions*! It 
doesn't mean "65% of CPU". It means 65% of the instructions.
  
  You are doing such good work with profiling and optimizing software, it would 
be so much better if you'd use the correct tools too!

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D12511

To: jtamate, #kate, cullmann, mwolff
Cc: mwolff, cullmann, #frameworks, michaelh, kevinapavew, ngraham, bruns, 
demsking, sars, dhaumann

D12511: optimization of KTextEditor::DocumentPrivate::views()

[Milian Wolff]

mwolff requested changes to this revision.
mwolff added a comment.
This revision now requires changes to proceed.


  lgtm in general, but codewise can be improved

INLINE COMMENTS

> katedocument.cpp:2835
>  m_views.insert(view, static_cast(view));
> +m_viewsCache = m_views.keys();
>  

just add the view, cf. the contains check above. no need to rebuild the full 
list here all the time

> katedocument.cpp:2852
>  m_views.remove(view);
> +m_viewsCache = m_views.keys();
>  

dito, just remove

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D12511

To: jtamate, #kate, cullmann, mwolff
Cc: mwolff, cullmann, #frameworks, michaelh, kevinapavew, ngraham, bruns, 
demsking, sars, dhaumann

D12511: optimization of KTextEditor::DocumentPrivate::views()

[Jaime Torres Amate]

jtamate updated this revision to Diff 33574.
jtamate added a comment.


  You are right, there is no change to m_views there.

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12511?vs=33047&id=33574

REVISION DETAIL
  https://phabricator.kde.org/D12511

AFFECTED FILES
  src/document/katedocument.cpp
  src/document/katedocument.h

To: jtamate, #kate, cullmann
Cc: cullmann, #frameworks, michaelh, kevinapavew, ngraham, bruns, demsking, 
sars, dhaumann

D12511: optimization of KTextEditor::DocumentPrivate::views()

[Christoph Cullmann]

cullmann requested changes to this revision.
This revision now requires changes to proceed.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D12511

To: jtamate, #kate, cullmann
Cc: cullmann, #frameworks, michaelh, kevinapavew, ngraham, bruns, demsking, 
sars, dhaumann

D12511: optimization of KTextEditor::DocumentPrivate::views()

[Christoph Cullmann]

cullmann added a comment.


  If you have benchmark results for that, I have no issues with the idea.
  Thought I think you only need to update the cache in add/removeView, not in 
KTextEditor::DocumentPrivate::createView

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D12511

To: jtamate, #kate
Cc: cullmann, #frameworks, michaelh, kevinapavew, ngraham, bruns, demsking, 
sars, dhaumann

D12511: optimization of KTextEditor::DocumentPrivate::views()

[Jaime Torres Amate]

jtamate created this revision.
jtamate added a reviewer: Kate.
Restricted Application added projects: Kate, Frameworks.
Restricted Application added a subscriber: Frameworks.
jtamate requested review of this revision.

REVISION SUMMARY
  Calculate the list of keys of a hash table is quite expensive.
  It is cheaper to keep that list and change it when a view is created or 
removed.

TEST PLAN
  Run kwrite and paste a relative long text several times and then undo until 
the text is empty, with only one view.
  Before: F5821740: kwrite_before.png  it 
used 65% of cpu in Kate::TextBuffer::notifyAboutRangeChange
  After: F5821746: kwrite_after.png  it 
uses 19% of cpu in in Kate::TextBuffer::notifyAboutRangeChange

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D12511

AFFECTED FILES
  src/document/katedocument.cpp
  src/document/katedocument.h

To: jtamate, #kate
Cc: #frameworks, michaelh, kevinapavew, ngraham, bruns, demsking, cullmann, 
sars, dhaumann