SymbolView: Avoid unneeded update of current item
ClosedPublic

Authored by loh.tar on Jul 26 2018, 6:56 PM.

Details

Summary
...to reduce a little bit CPU use

- Don't update current item while ducument is editied 
- Remember current edit line to avoid update
Test Plan

Sadly can't I right now find a file and a way to edit to reproduce these flickiering :-S (?)

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
loh.tar created this revision.Jul 26 2018, 6:56 PM
Restricted Application added a project: Kate. · View Herald TranscriptJul 26 2018, 6:56 PM
Restricted Application added a subscriber: kwrite-devel. · View Herald Transcript
loh.tar requested review of this revision.Jul 26 2018, 6:56 PM
loh.tar edited the test plan for this revision. (Show Details)Jul 27 2018, 1:21 AM
sars added a comment.Jul 27 2018, 6:33 AM

Hmm... I'm not totally convinced ;)

Stopping the m_currItemTimer when we have an edit is good! We could also avoid starting the timer in cursorPositionChanged() if m_updateTimer is active.

What I'm not convinced about is the increasing of the m_currItemTimer timeout or to cache the first & last lines of the currently active item.

Reasons:

  1. The timer is restarted and not triggered as long as the cursor moves. It triggers only when the cursor has stopped for over 100ms. The limit could be just a bit higher than the key-repeat delay and we would just get two newActveItem() invocations. One at the start before the key-repeats have started and one at the end when the key is released.
  1. I hope you don't usually have files with more than 1000 "symbols" and even if you would have 1000 symbols the time for looping through those would not be noticeable.

Try adding a QElapsedTimer at the start of updateCurrTreeItem() and check at the end how much time it took. I bet for most files it does not ever go over 1ms (except if there is a kernel-scheduling pause)

It reads for me only like , "The CPU waste does not matter."
I'm very disappointed in this analysis.

No word why this patch make more problems than it solves. Your argument, "it take only 1ms", is probably true, but why then not remove the delay timer completely?
You see, it's always good practice to save resources.

The caching of the first&last line is the trick where I be especially "proud of". It cost almost nothing and save so much with absolutely no drawback.

Without this patch I noticed 5% CPU consumption only with moving the cursor, with this patch I see almost no amplitude.

Regarding the timer settings, that may be debatable but also a good source of dispute. I have played around with that and found, so I think, a good compromise.

sars added a comment.Jul 27 2018, 9:56 AM

It definitely does not mean "The CPU waste does not matter".

I actually tried adding the timer to the function to measure the time. I tried it on a almost 4000 lines long C++ file (far too big ;) and it has around 200 symbols. In the measurements I did, it never reached a full millisecond to finish, it always printed 0ms.

I'm just saying that I don't think the complexity added for the micro-optimization is worth it. It just becomes harder to maintain. The possible gain of less than 1ms in the situation when the user presses a key or releases the key, just does not change the perception of how fast or slow the application reacts.

Making the delay longer (200 ms) does not decrease the number of times the function is called. It still stays at once when the key is pressed and once when the key is released.
The bad thing is that the user actually feels that the plugin is slower as it now takes 200ms for the current-item to change in stead of just 100ms previously.

That said I think stopping the m_currItemTimer on document change and not starting it if m_updateTimer is active, is a real noticeable improvement. Especially when adding new lines just above a symbol.

One thing that you could cache without much complexity is the last cursor position and only start the timer if the line changes.

Regards,

Kåre

If this patch reduces the CPU usage during this operation from 5% to essentially 0%, that seems like a worthwhile change for the improvement to battery life quite aside from user perceptions of speed. Is there something we can do to improve the diff to make it more merge-worthy, @sars?

sars added a comment.Jul 27 2018, 2:12 PM

You cannot measure the CPU consumption of this particular function by looking at the "System Activity" while scrolling up and down the file. There are a million other code paths that get executed while you scroll the page. System Activity gives you an average CPU load of the application during 2000ms. This less than 1ms function only gets called once when you start scrolling and once when you stop.

What this patch does is that it avoids a for-loop of normally less than 100 elements, if you only scroll a couple of lines at a time.

Sorry, I'm not convinced before I get a trace from a profiler like hotspot, that clearly indicates that this patch would have any noticeable effect.

Thanks, that was much better.

It seams we have different priorities or tastes when is something an acceptable waste or effort.

  • Your point is, as long as everything still works fluffy enough there is no need to change something.
  • My point is, avoid as much work (at CPU side, not at coder side) as possible, do only things once and only when there is a need for.

I actually tried adding the timer to the function to measure the time. I tried it on a almost 4000 lines long C++ file (far too big ;) and it has around 200 symbols. In the measurements I did, it never reached a full millisecond to finish, it always printed 0ms.

Sadly ignored you my CPU use observation and still arguments with these less than 1ms. Would you say you could not reproduce my observation that would that be a point.

I'm just saying that I don't think the complexity added for the micro-optimization is worth it. It just becomes harder to maintain.

That is sadly not a hard fact, its a matter of taste when is something too complex. With that can you almost ever reject some request. When I look at the changes appears they not so complex to me. The existing quirk, how the item is searched, was already before such inscrutable. Sorry, not my code.

In the meantime I have an idea how to solve that something simpler, but not really clean. The main problem is, that you can't sort the tree by there line number column due to string compare -> 1 10 2 20. So my idea is that the line numbers, after the tree is new finished, to pad with "0" to become 0001 0002 0010 0020. But then must be made a decision how many lines to support.

Making the delay longer (200 ms) does not decrease the number of times the function is called.

Which function? cursorPositionChanged() of cause not, updateCurrTreeItem() absolutely, as long as the user is active.

It still stays at once when the key is pressed and once when the key is released.

It seams you mean cursorPositionChanged(),that's irrelevant. The timer is restarted and updateCurrTreeItem() only called once.

The bad thing is that the user actually feels that the plugin is slower as it now takes 200ms for the current-item to change in stead of just 100ms previously.

Oh, you should try me other patch, there IS it notable, but here I have doubts.

One thing that you could cache without much complexity is the last cursor position and only start the timer if the line changes.

With that I started, then I thought we can do it better.

Greetings, Lothar

You cannot measure the CPU consumption of this particular function by looking at the "System Activity" while scrolling up and down the file.

did not light me

There are a million other code paths that get executed while you scroll the page.

Of cause

System Activity gives you an average CPU load of the application during 2000ms. This less than 1ms function only gets called once when you start scrolling and once when you stop.

But these is now gone (?!)

Sorry, I'm not convinced before I get a trace from a profiler like hotspot, that clearly indicates that this patch would have any noticeable effect.

It really surprised me that you fight so much against this little patch

Loh, arguing aggressively is not productive. It just makes everybody gear up for a battle and we stop listening to one another. Let's instead focus on how we move forward. @sars has requested a hotspot trace that proves the issue; can you provide that?

Loh, arguing aggressively is not productive.

I'm sorry, that it sounds so. I have only tried to to argue soberly and keep emotions away

It just makes everybody gear up for a battle and we stop listening to one another.

Once more, sorry please! No interest in some battle

@sars has requested a hotspot trace that proves the issue; can you provide that?

I don't think so, sorry. Never heard before.

brauch added a subscriber: brauch.Jul 27 2018, 3:29 PM

Just install perf and hotspot, then compile in "RelWithDebInfo" mode and run

perf record -g kate
[do stuff]
[quit]
hotspot perf.data

and look at e.g. the flame graph. You may want to filter by time in the bottom bar to get rid of the startup. Stuff that does not show up there as a major time slice is probably not worth optimizing.

Thanks for the hint.

Right now I'm not in the mood to do this, and a look at the hotspot dependencies in AUR make it even worse.

Thanks for the hint.

Right now I'm not in the mood to do this, and a look at the hotspot dependencies in AUR make it even worse.

Hmm, why though? I just looked, it seems to depend on almost nothing KDevelop doesn't depend on anyways.

But if you are interested in performance, which you seem to be, this is something I would really recommend to look into anyways. Once you got used to using a proper profiler, you'll never want to go back to ksysguard and QTimer ever again.

That said, I can understand your mood (I've felt the same before with things I submitted somewhere, I think everyone has) and I'm sorry for it. But I agree with Kore that we should try to merge patches based on what objectively makes things better.

I can only confirm that flamegraphs are really great in helping to really see the hotspots one need to take care off.

If you can't stand to install more tools than perf, you can generate them with the perl scripts from https://github.com/brendangregg/FlameGraph, too, with no dependencies and look at the SVG in your favorite browser.

Thanks to both of you, just your interest and encouragement :-)

Regarding dependencies: I use rarely the AUR and use no helper.
There are 14 dependencies listed, some of them are of cause already installed but is looks anyway deterrent.

Thanks for the hotspot substitute tip, I may try it some day.

Currently, I can't reproduce the benefit of this patch with clearness and that make my mood once more not better :-/
And probably is the most useful part of it the stopped timer, which Kåre quickly recognized.

But, I still like the idea of it and still find the code not so terrible to reject him.

loh.tar updated this revision to Diff 38657.Jul 28 2018, 2:59 PM
loh.tar edited the summary of this revision. (Show Details)
loh.tar edited the test plan for this revision. (Show Details)

I would still prefer the original patch but must agree that the benefit is not as much as I thought.
I hope this version is now acceptable.

  • Reduce the check to only the current line as suggested
  • Don't change delay timer to avoid possible issue of dispute
  • Move m_symbols check where it should be done
  • Call slotDocEdited() instead of slotRefreshSymbol() in slotDocChanged() to avoid parsing twice

Making the delay longer (200 ms) does not decrease the number of times the function is called.

Which function? cursorPositionChanged() of cause not, updateCurrTreeItem() absolutely, as long as the user is active.

I have to apologize, I was totally wrong there. It is as you said. Sorry.
Even more sorry that I forget that and add a to my recent update a comment about the timers.
Shame on me.
Lothar

Call slotDocEdited() instead of slotRefreshSymbol() in slotDocChanged() to avoid parsing twice

Sigh. This cause in conjunction with D14409 a ugly, a not acceptable, delay.

sars added a comment.Jul 29 2018, 7:22 AM

What I still would like to have in this patch is the preventing of the m_currItemTimer from being started while m_updateTimer is active.

If you press enter a couple of times just above a symbol, you notice that the cursor position is updated and indicates that you have moved to the symbol below, but then after a little while, the symbols are updated and the active item is then again updated. The first update is useless and a bit annoying ;)

addons/symbolviewer/plugin_katesymbolviewer.cpp
179

I like this change! :)

Checking pointers should be done close to where you use them.

I think we even could merge slotRefreshSymbols() with parseSymbols() as the first now just calls the later.

199

As you noticed this change is not good. It does not decrease the number of calls to parseSymbols() it just adds a delay of 500ms. Notice that cursorPositionChanged and slotDocEdited are connected later in this slot and therefore not trigger any calls to them.

What I still would like to have in this patch is the preventing of the m_currItemTimer from being started while m_updateTimer is active.

Nice, think see what you mean. Stay tuned.

addons/symbolviewer/plugin_katesymbolviewer.cpp
179

I think we even could merge slotRefreshSymbols() with parseSymbols() as the first now just calls the later.

Is that a request to do it in this patch? Then I will.

199

Yes, will revert this

loh.tar updated this revision to Diff 38696.Jul 29 2018, 9:53 AM
loh.tar edited the summary of this revision. (Show Details)
loh.tar edited the test plan for this revision. (Show Details)

For your convenience with active debug lines, will remove them after a go and before landing

sars added a comment.Jul 29 2018, 11:31 AM

We can have the slotRefreshSymbol() merging in a different patch.

Starts to be ready for committing!

I was going to say that the parseSymbols() function is very cheap compared to for example the mini-map updating, but hotspot did not agree ;) The mini-map is a bit more expensive and is updated 500 ms after the last edit.

I wonder where the second parseSymbols() call comes from on startup, but when you just switch document you only get one parseSymbols() and one updateCurrTreeItem() which is good :)

That said I don't think the user will notice an improvement in "performance" or battery time by making the timeout 1000ms in stead of 500. The user would notice tho that the update comes later and think/feel like it is slower and think Kate is just badly optimized ;)

Remove the debug printouts and some of the unneeded comments and we are ready with this one :)

loh.tar updated this revision to Diff 38700.Jul 29 2018, 12:36 PM

Remove the debug printouts and some of the unneeded comments and we are ready with this one :)

Not clear to me which one is "unneeded" for you, so I keep some of my new and did not remove a couple of old out commented debug lines

I was going to say that the parseSymbols() function is very cheap compared to for example the mini-map updating, but hotspot did not agree ;) The mini-map is a bit more expensive and is updated 500 ms after the last edit.

Thanks you do it on your own, these hotspotting. I still don't like to fumble with new things right now, but I'm anyway now curious to get some hard facts of your test. No full report, just only three numbers. How long need, or how much cost, mini-map, parse-symbols and update-item?

If there would be a "edit-is-finished" signal then could that make global adjustable to fit users taste. Guess, somthing you find not important

sars accepted this revision.Jul 30 2018, 5:56 AM
This revision is now accepted and ready to land.Jul 30 2018, 5:56 AM
sars added a comment.Jul 30 2018, 6:19 AM

Sorry!! forgot to add you as the author when I pushed :(

This revision was automatically updated to reflect the committed changes.

Sorry!! forgot to add you as the author when I pushed :(

Happens to me right after the trouble I have caused with this.
Not your fault, it's that stupid Phabricator.
And due to the good find by you to block m_currItemTimer it's somehow fair. :-)