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

perf: gather all event listeners in parallel #1667

Merged
merged 2 commits into from
Feb 8, 2017
Merged

Conversation

brendankenny
Copy link
Member

@brendankenny brendankenny commented Feb 8, 2017

this was some low hanging performance fruit discussed last week, getting all the event listeners for all nodes in the page in parallel rather than waiting on each command to finish. We aren't enabling/disabling domains or listening for events, so this should be safe.

Profile for the EventListeners gatherer doesn't look all that different but it does save e.g. 700ms (1300 -> 600) when running on www.chromestatus.com.

before:
screen shot 2017-02-07 at 17 09 05

after:
screen shot 2017-02-07 at 17 09 20

(purple block under 11750ms is all the commands now being sent at once. Remaining scatter of blocks is now just returning messages, not interleaved returning messages and sending messages)

Copy link
Contributor

@ebidel ebidel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small changes. Big/medium wins. 🏆

@brendankenny
Copy link
Member Author

sneaky post-approval commit: @paulirish reminded me we had also discussed disabling websocket compression since our payloads are so tiny and the vast majority of the time in sendCommand and its callbacks is spent compressing and decompressing messages. So I've disabled it.

However, the win is basically imperceptible. The time spent per command has dropped, but the execution time of all commands was so small the reduction had little effect on overall execution time.

However however, the ws readme recommends:

The [permessage-deflate] extension is enabled by default but adds a significant overhead in terms of performance and memory consumption. We suggest to use WebSocket compression only if it is really needed.

and there may come a time where we need a gatherer that runs many commands in a row that can't be run in parallel, so leaving compression off as the new default makes sense.

@ebidel
Copy link
Contributor

ebidel commented Feb 8, 2017

🚫 🏋️

@brendankenny brendankenny merged commit c6aeb33 into master Feb 8, 2017
@brendankenny brendankenny deleted the parallellisteners branch February 8, 2017 02:11
@paulirish
Copy link
Member

LGTM! so good

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.

None yet

3 participants