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

Performance: use requestIdleCallback for lazy execution #4259

Merged
merged 9 commits into from
Aug 25, 2017
Merged

Conversation

kepta
Copy link
Collaborator

@kepta kepta commented Aug 22, 2017

This pull request adds the ability to schedule callbacks smartly. Currently, all of the callbacks are executed with same priority. We need a way to prioritise them. Fortunately browsers have reached a consensus and given us an API called requestIdleCallback exactly for this.

I have added a shim for browsers which don't support it. On paper the performance should not degrade for browsers which fallback to shim, since it is a simple timeout.

Things improved.

  1. Ignore duplicate entities
    iD simply parses all the entities and sometimes they happen to be already parsed. This becomes a problem when dealing with fat relations/ways, which span across multiple bounding boxes and iD has to waste time parsing them again. In this PR, I have added a check for entities which have already been parsed.

  2. Scheduling parsing as a low priority task
    Javascript handles fetch request async, but the problem is when the data reaches the client and the client has to parse it. For e.g. you are panning the map and the javascript decides to parse the data right away. This leads to a stutter and lag for the end user.
    To mitigate this, fetching and parsing have been converted to a low priority task, (here and here) which drastically improves the responsiveness of the application.

  3. Parsing in small chunks
    Even if we schedule a task as low priority, when it executes for a long time it will cause the application to stutter. The way out of this would be to break it into smaller chunks and use the IdleDeadlline API to figure out when to stop and resume the work. Relevant code here and here.

  4. Scheduling the redraw
    iD's redraw is one fat function and if it gets executed at the wrong time, there is a noticeable stutter.
    The solution here is to wrap it within and idleCallback, so that the timing is scheduled properly.

  5. Replacing throttles and debounces with requestIdleCallback
    I have naively added requestIdleCallback to some part of the code, labels.js, background.js,
    I am not sure if it fits well :P @bhousel ?

Other things which can be improved

Layer rendering
I tried to break these guys, but failed. The idea is to schedule them as a low priority task. Which would drastically improve iD's responsiveness. But they are too coupled and iD simply acts like drunk if I try to call them lazily.

svgTagClasses
I find this code quite confusing and complex. It also takes some 200ms to compute. We should break it down or optimise it.

Profile comparison
Over here I have compared the loading profile before and after. Notice the smaller chunks efficiently spaced.
compare_id_profile

Comparison Optimized version vs Current iD master

@kepta kepta added the wip Work in progress label Aug 22, 2017
@kepta
Copy link
Collaborator Author

kepta commented Aug 24, 2017

@bhousel this is good for review.

@kepta kepta removed the wip Work in progress label Aug 24, 2017
@bhousel
Copy link
Member

bhousel commented Aug 24, 2017

Wow this looks excellent, and the changes do make the app much more responsive!

From the "to be improved" section:

  • async layer rendering might not be possible, I'm not sure. The layers "act like drunk" because they're being drawn with a slightly wrong projection, which then catches up whenever the next non-differenced redraw happens.

  • svgTagClasses definitely should not be taking 200ms to execute, so we can look into that as a separate issue. That code is kind of complicated because osm tags are complicated.

@bhousel bhousel changed the title Performance Performance: use requestIdleCallback for lazy execution Aug 24, 2017
@bhousel
Copy link
Member

bhousel commented Aug 24, 2017

The test failures for loadEntity and loadEntityVersion make sense. I think I can fix it..

@bhousel
Copy link
Member

bhousel commented Aug 25, 2017

This is working ok now. I did some editing with it today and will do a bit more tonight just to test. It seems good in Chrome, Firefox, and Safari. I'll double check with Edge and IE (but if PhantomJS tolerates the shim, I'm sure we are in good shape).

I'm really excited about this for a few reasons. One is because parts of iD got a lot faster, but even more than that, it opens up a lot more opportunities to improve performance - there are still a bunch of easy wins.

  • feature filtering and classification can definitely be deferred (this is a sizeable chunk of time in redraw())
  • we haven't added this yet to other services like nominatim or mapillary
  • can background load large multipolygons to complete them
  • can think about how defer the layer rendering
  • of the render functions, labels are taking up a lot more time than they should - we can defer/cache label placement instead of redoing it each time.

@bhousel bhousel merged commit c977f15 into master Aug 25, 2017
@bhousel bhousel deleted the performance branch August 25, 2017 02:13
@JamesKingdom
Copy link
Collaborator

@kepta This is amazing 😮 , it feels so much more responsive 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants