Skip to content
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
Closed

Slow Gesture/BaseActionItem #37917

jrieken opened this issue Nov 9, 2017 · 11 comments
Assignees
Labels
debt Code quality issues on-testplan perf-startup release-notes Release notes issues
Milestone

Comments

@jrieken
Copy link
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
Copy link
Member Author

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
Copy link
Member

bpasero commented Nov 9, 2017

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

@isidorn
Copy link
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 Code quality issues label Nov 9, 2017
@alexdima
Copy link
Member

alexdima commented Nov 9, 2017

@bpasero I don't know

@jrieken
Copy link
Member Author

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

@roblourens
Copy link
Member

@isidorn
Copy link
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
Copy link
Member Author

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
Copy link
Member

bpasero commented Nov 20, 2017

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

@jrieken
Copy link
Member Author

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
Copy link
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 as completed Nov 22, 2017
@isidorn isidorn mentioned this issue Nov 23, 2017
2 tasks
@isidorn isidorn added on-testplan release-notes Release notes issues labels Dec 8, 2017
@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.
Labels
debt Code quality issues on-testplan perf-startup release-notes Release notes issues
Projects
None yet
Development

No branches or pull requests

5 participants