[performance] Replace element.offsetHeight

[aminya]

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!

  drawGutter () {
    this.element.classList.remove('gutter-top', 'gutter-bottom');
        
    const scrollHeight = this.element.scrollHeight;

      if (this.element.scrollTop > 0) {
        this.element.classList.add('gutter-top');
      }
      if (this.element.scrollTop < scrollHeight) {
        this.element.classList.add('gutter-bottom');
      }
}

I see other people have the same issues, but there is no solution devrelm/resize-observer#5

[ericcornelissen]

Would seem intuitively true, did you check that this does not reintroduce #156?

[aminya]

@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.

[suda]

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

[aminya]

Making it optional with default option being false seems good.

However, isn't there any way to not use element.offsetHeight for the optional settings?

[ericcornelissen]

However, isn't there any way to not use element.offsetHeight for the optional settings?

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?

[ericcornelissen]

Making it optional with default option being false seems good.

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...

[aminya]

Making it optional with the default option being false seems good.

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

[ericcornelissen]

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 drawGutter() 🤔 (regardless of whether or not the activationHooks is present in package.json)

[aminya]

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

I added some speed related descriptions in the option. If we can add a better solution, we can return back to 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 drawGutter() 🤔 (regardless of whether or not the activationHooks is present in package.json)

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.

[ericcornelissen]

I can verify that the performance.now() gives the indicated results (checked out on 48cd604). However, this performance increase is not at all reflected in the load time of the package as reported in the Atom settings.

Setup	Start Atom, Package load time	Reload Atom, Package load time
With hook, original drawGutter	~15ms	~15ms
With hook, empty drawGutter	~15ms	~15ms
No hook, original drawGutter	~100ms	~100ms
No hook, empty drawGutter	~100ms	~100ms

Are you seeing different results @aminya? Can you perhaps share your settings for the tool-bar package?

@suda, did you benchmark this as well?

[aminya]

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 master:

On 48cd604

My settings:

[ericcornelissen]

On Rollup ...

For this discussion I'm not interested in how Rollup affects the loading time.

For proper benchmarking, you should remove the hook.

I did, I shared my results both with and without the activation hook...

I had similar problems with people seeing the different results on their machines.

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 master to 48cd604 I'm not seeing a notable difference (basically the same results as I reported earlier) 🤔

Also, it doesn't seem like your results line up perfectly with the 30ms (86-62=24) you reported in the original post (or the ~50ms I'm getting at my end). Are you sure these are not just random variations in load time? Did you reload or restart Atom multiple times to see if there is any variation (both on master and on 48cd604).

Also, from the thread you linked: are you measuring these results in the latests stable version of Atom (i.e. v1.39.x) or some nightly version? And which OS are you doing these tests on? (I'm currently testing on Linux, I can run tests on Windows as well later.)

[aminya]

Yes for benchmarking I reload Atom a couple of times. That is strange. Anyways, I trust windows.performance.now more than anything else. If that says that we are doing better, I get happy.

For your information, I am on Windows and this is my system (I have a fast SATA drive) https://www.userbenchmark.com/UserRun/28980831

[ericcornelissen]

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.

Yes for benchmarking I reload Atom a couple of times. That is strange. Anyways, I trust windows.performance.now more than anything else. If that says that we are doing better, I get happy.

I trust windows.performance.now as well, but what we're optimizing for is what Atom says is the amount of time this package contributes to the startup time. If these 30ms we're saving do not actually make Atom load faster, we're not achieving what we set out to do. That is, it does not at all help towards making Atom with tool-bar look "less bulky". Moreover, the note for the "Use Gutter" option from #305 is misleading to the user is it quiet possibly doesn't affect load times.

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!}