-
Notifications
You must be signed in to change notification settings - Fork 55
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[performance] Replace element.offsetHeight #303
Comments
Would seem intuitively true, did you check that this does not reintroduce #156? |
@ericcornelissen Removing draw gutter solves both the performance and shadowing problem. I don't think we need to sacrifice the performance for something very minor. I can't even see what this gutter is doing. |
The gutter shadow indicates that there are more icons in the tool-bar even if they are not visible. It will change if the window changed its size. I would suggest making it optional so users have the choice of turning it off :) |
Making it optional with default option being false seems good. However, isn't there any way to not use |
I would suspect not, as its value is probably only available after the DOM has loaded. Perhaps we can look into executing this function in a non-blocking way (using Promises or workers) so that it doesn't affect the package load time significantly? |
For what it's worth. I don't think this solution would justify merging #300 as it would require the end-user to invest a significant amount of time relating disabling the gutter to improving load time... |
Here I meant no gutter by default. 😁 See #305 |
My argument still holds, if someone "needs" to enable the option it's unfair to punish them with worse loading times (at least without explaining this). For what it's worth, when running some tests regarding this issue at my end I'm not seeing an improvement in loading them when removing the code inside |
I added some speed related descriptions in the option. If we can add a better solution, we can return back to this.
That is strange. You can benchmark using this if atom is not working properly: let t1 = window.performance.now();
this.drawGutter()
console.log( window.performance.now() - t1) Add it to the first drawGutter call. |
I can verify that the
Are you seeing different results @aminya? Can you perhaps share your settings for the tool-bar package? @suda, did you benchmark this as well? |
For proper benchmarking, you should remove the hook. Atom sometimes acts weird in terms of measuring the loading time. I had similar problems with people seeing the different results on their machines. On Rollup #302: On 48cd604 |
For this discussion I'm not interested in how Rollup affects the loading time.
I did, I shared my results both with and without the activation hook...
If that is the case, perhaps it is something going wrong at your end. This is also why I would like to know what @suda is seeing at their end. Interesting, even if I compare Also, it doesn't seem like your results line up perfectly with the Also, from the thread you linked: are you measuring these results in the latests stable version of Atom (i.e. |
Yes for benchmarking I reload Atom a couple of times. That is strange. Anyways, I trust For your information, I am on Windows and this is my system (I have a fast SATA drive) https://www.userbenchmark.com/UserRun/28980831 |
Even more interesting! On Windows I'm seeing even less of a difference between master and 48cd604.
I trust This does not mean that #305 is necessarily invalid, it reduces the CPU load when starting Atom perhaps? That said, even if it is valid: I still don't like having the option for a 30ms performance improvement that the end user won't notice to disable a feature that I would say is quite important if you have a toolbar with many items (or just a small screen). In conclusion: given that you already had some mismatch in load times in the issue you linked I highly suggest we wait for the benchmarking results from @suda (or anyone else for that matter!) and decide what to do accordingly. which, for the record, may be that we leave the changes of #305 as they are! |
I found an interesting thing: This line of code takes 30ms for its execution! Can't we do something about this?!
This is only used for comparing it with
scrollHeight
. Can't we assume that this is true all the time?https://github.com/suda/tool-bar/blob/master/lib/tool-bar-view.js#L185-L195
I disabled all the drawGutters and the loading time is greatly reduced! We should fix this line of code.
I changed the code to this and it works without any issues and the performance issue is gone!
I see other people have the same issues, but there is no solution devrelm/resize-observer#5
The text was updated successfully, but these errors were encountered: