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

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

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

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

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

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

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

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.

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

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

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

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

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

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.

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

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

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

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

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

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(); >

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

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=33574 REVISION DETAIL https://phabricator.kde.org/D12511 AFFECTED

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

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()

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

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

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