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

Slow Gesture/BaseActionItem #37917

Closed
jrieken opened this Issue Nov 9, 2017 · 11 comments

Comments

Projects
None yet
5 participants
@jrieken
Member

jrieken commented Nov 9, 2017

  • Profile a startup/reload, sort profile by heaviest calls
  • The DomListener-type is a heavy hitter and takes ~45ms of which ~80% are spend in setting up the Gesture

screen shot 2017-11-09 at 11 12 46

CPU-20171109T111657.cpuprofile.txt

It seems that adding the touch{start/move/end/} events takes a lot longer than other event listener... Suggestions:

  • don't have a listener per BaseActionItem but per container (action bar)
  • disable gesture when target device does support touch
@jrieken

This comment has been minimized.

Show comment
Hide comment
@jrieken

jrieken Nov 9, 2017

Member

Not sure who owns this... I started this 5 years ago but I don't feel responsible for it.

Member

jrieken commented Nov 9, 2017

Not sure who owns this... I started this 5 years ago but I don't feel responsible for it.

@bpasero

This comment has been minimized.

Show comment
Hide comment
@bpasero

bpasero Nov 9, 2017

Member

Is there any way to find out if the device is touch enabled or not. @alexandrudima ?

Member

bpasero commented Nov 9, 2017

Is there any way to find out if the device is touch enabled or not. @alexandrudima ?

@isidorn

This comment has been minimized.

Show comment
Hide comment
@isidorn

isidorn Nov 9, 2017

Contributor

I can look into this.

Contributor

isidorn commented Nov 9, 2017

I can look into this.

@isidorn isidorn added this to the November 2017 milestone Nov 9, 2017

@isidorn isidorn added the debt label Nov 9, 2017

@alexandrudima

This comment has been minimized.

Show comment
Hide comment
@alexandrudima

alexandrudima Nov 9, 2017

Member

@bpasero I don't know

Member

alexandrudima commented Nov 9, 2017

@bpasero I don't know

@jrieken

This comment has been minimized.

Show comment
Hide comment
@jrieken

jrieken Nov 9, 2017

Member

'ontouchstart' in window and some extra checks could do it, see https://stackoverflow.com/questions/14439903/how-can-i-detect-device-touch-support-in-javascript.

I have tested the above check on my MBP and X1 with the results one would expect...

Member

jrieken commented Nov 9, 2017

'ontouchstart' in window and some extra checks could do it, see https://stackoverflow.com/questions/14439903/how-can-i-detect-device-touch-support-in-javascript.

I have tested the above check on my MBP and X1 with the results one would expect...

isidorn added a commit that referenced this issue Nov 20, 2017

@isidorn

This comment has been minimized.

Show comment
Hide comment
@isidorn

isidorn Nov 20, 2017

Contributor

@jrieken I have pushed a potential fix for this, however the results are not that much different than your initial run
Maybe you could get latest and re-run to double check?

I double checked if the touch check works on my mac (false) and x1(true) and all is good.
Also to note is that I made the isTouchDevice a const so in this version that check would be done once on startup instead of making it a func and checking every time.

screen shot 2017-11-20 at 16 56 31

Contributor

isidorn commented Nov 20, 2017

@jrieken I have pushed a potential fix for this, however the results are not that much different than your initial run
Maybe you could get latest and re-run to double check?

I double checked if the touch check works on my mac (false) and x1(true) and all is good.
Also to note is that I made the isTouchDevice a const so in this version that check would be done once on startup instead of making it a func and checking every time.

screen shot 2017-11-20 at 16 56 31

@jrieken

This comment has been minimized.

Show comment
Hide comment
@jrieken

jrieken Nov 20, 2017

Member

Maybe you could get latest and re-run to double check?

No, the slow bit is creating Gesture-objects and when you just prevent that from one user, all others will still be slow... and you still do that

Member

jrieken commented Nov 20, 2017

Maybe you could get latest and re-run to double check?

No, the slow bit is creating Gesture-objects and when you just prevent that from one user, all others will still be slow... and you still do that

@bpasero

This comment has been minimized.

Show comment
Hide comment
@bpasero

bpasero Nov 20, 2017

Member

Can we fold this into Gesture itself so that every client benefits from this performance fix?

Member

bpasero commented Nov 20, 2017

Can we fold this into Gesture itself so that every client benefits from this performance fix?

@jrieken

This comment has been minimized.

Show comment
Hide comment
@jrieken

jrieken Nov 20, 2017

Member

There following problems should be tacked (ideally in this order)

  1. don't create too many gesture object as its creation is slow, e.g. only create one per action bar not per action bar item. maybe just one for the whole document to share?
  2. revisit using Gesture for click/tap events. That's the main usage and questionable
  3. diable gestures for devices without touch
Member

jrieken commented Nov 20, 2017

There following problems should be tacked (ideally in this order)

  1. don't create too many gesture object as its creation is slow, e.g. only create one per action bar not per action bar item. maybe just one for the whole document to share?
  2. revisit using Gesture for click/tap events. That's the main usage and questionable
  3. diable gestures for devices without touch
@isidorn

This comment has been minimized.

Show comment
Hide comment
@isidorn

isidorn Nov 22, 2017

Contributor

Fixed via #38946
Seems like we save around 40ms on startup.

I verified all the touch behavior works as before including the balistic scroll. Just fyi @joaomoreno @alexandrudima in case somebody reports touch scroll issues in the editor or the tree that you assign to me.

screen shot 2017-11-22 at 12 55 54

Contributor

isidorn commented Nov 22, 2017

Fixed via #38946
Seems like we save around 40ms on startup.

I verified all the touch behavior works as before including the balistic scroll. Just fyi @joaomoreno @alexandrudima in case somebody reports touch scroll issues in the editor or the tree that you assign to me.

screen shot 2017-11-22 at 12 55 54

@isidorn isidorn closed this Nov 22, 2017

@isidorn isidorn referenced this issue Nov 23, 2017

Closed

Test: touch support #39021

2 of 2 tasks complete

@vscodebot vscodebot bot locked and limited conversation to collaborators Jan 6, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.