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

Worker transfer benchmark #7145

Merged
merged 5 commits into from
Aug 17, 2018
Merged

Worker transfer benchmark #7145

merged 5 commits into from
Aug 17, 2018

Conversation

mourner
Copy link
Member

@mourner mourner commented Aug 17, 2018

A new benchmark to measure JSON worker transfer costs for PRs like #7124, #7132 and #7127.

I'm gathering parsed tile/glyph/icon data, serializing it, removing all array buffers (because we can't transfer them repeatedly), and then sending this payload over and over to an empty worker and back to measure the structured cloning impact.

It may be a little contrived, but I don't see any other good way to benchmark this.

We also may need to pick a more representative set of tiles (e.g. a set of dense z11 ones).

screen shot 2018-08-16 at 17 17 08

@mourner mourner requested a review from jfirebaugh August 17, 2018 00:21
Copy link
Contributor

@jfirebaugh jfirebaugh left a comment

Choose a reason for hiding this comment

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

This seems like a good approach to me. Do you think it's worth including the cost of serialize/deserialize in the benchmark as well, or try to scope it to just the browser overhead as much as possible?

});
}

bench(getGlyphs: Function = (params, callback) => callback(null, this.glyphs[JSON.stringify(params)]),
getImages: Function = (params, callback) => callback(null, this.icons[JSON.stringify(params)])) {
parseTiles(loadImages: Function, loadGlyphs: Function) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than using inheritance for code reuse, could we pull parseTiles out into a module in bench/lib that is used by Layout and WorkerTransfer?

Copy link
Member Author

Choose a reason for hiding this comment

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

We could, but I think I'd prefer inheritance in this case — it's easier when all the actual benchmark code is in one folder, and we might want to customize parseTiles in other benchmarks. LayoutDDS also inherits Layout and rewriting it to use composition or util methods for consistency will be very cumbersome.

}

function barePayload(obj) {
// strip all transferables from a worker payload
Copy link
Contributor

Choose a reason for hiding this comment

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

Mention the reason why here too.

@mourner
Copy link
Member Author

mourner commented Aug 17, 2018

Do you think it's worth including the cost of serialize/deserialize in the benchmark as well, or try to scope it to just the browser overhead as much as possible?

I wanted to scope it to main thread overhead, because it's a primary cause of stutter. Costly serialization isn't necessarily bad — it doesn't affect the main thread much, and e.g. in #7132, it is used to index a data structure before passing it to the main thread, getting fully transferable payload in return. We might try to include deserialization though, because it happens on the main thread.

@jfirebaugh
Copy link
Contributor

We already have a convention for shared benchmark code though -- bench/lib contains a number of shared utility functions. See create_map.js, create_style.js, etc.

@mourner
Copy link
Member Author

mourner commented Aug 17, 2018

@jfirebaugh true, although I still don't see how I could cut out parseTiles without making the code more confusing and less extensible. Maybe let me wrap up the PR without the refactoring, and then you'll show me with a commit to it or a follow-up PR?

@mourner mourner requested a review from jfirebaugh August 17, 2018 23:30
Copy link
Contributor

@jfirebaugh jfirebaugh left a comment

Choose a reason for hiding this comment

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

Thanks! This will be a valuable addition to the suite.

Copy link
Contributor

@jfirebaugh jfirebaugh left a comment

Choose a reason for hiding this comment

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

Thanks! This will be a valuable addition to the suite.

@mourner mourner merged commit c582e64 into master Aug 17, 2018
@mourner mourner deleted the worker-transfer-bench branch August 17, 2018 23:45
@ansis
Copy link
Contributor

ansis commented Aug 18, 2018

removing all array buffers (because we can't transfer them repeatedly)

Is it possibly to make multiple copies of the instead of removing them? There might be some overhead to transferring arrays and it could be good to capture that. But maybe that isn't necessary right now

@mourner
Copy link
Member Author

mourner commented Aug 18, 2018

@ansis I assumed it's not possible because we don't know beforehand how many times the bench will run. Maybe @jfirebaugh has ideas about that?

@jfirebaugh
Copy link
Contributor

Yeah, the number of runs is not deterministic (depends on the measured runtime). I think this solution is fine.

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