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

Pack Geometries before passing to a web worker #2342

Merged
merged 26 commits into from
Jan 7, 2015
Merged

Conversation

bagnell
Copy link
Contributor

@bagnell bagnell commented Dec 23, 2014

I'm opening this PR early so @mramato can take a look. I'll post performance numbers soon.

});
if (defined(geometry.constructor.pack)) {
var packedLength = defined(geometry.constructor.packedLength) ? geometry.constructor.packedLength : geometry.packedLength;
var array = new Float64Array(packedLength);
Copy link
Contributor

Choose a reason for hiding this comment

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

I need to dive into this a bit; but I suspect in order to see any real performance increase we'll need to refactor this code that we only create numberOfCreationWorkers typed arrays instead of creating a typed array per sub-task. Currently, it looks like we are subdividing the tasks after creating a single array, rather than figure out how many should go in each task and doing it up front.

This problem is starting to come back to me now that I'm looking at it; in addition to packing I think we need to overhaul the way the creation tasks are sent to the worker as well. The overhead of creating a promise and task per geometry is probably larger than we need it to be. I don't know how familiar you are with all of the worker stuff, but I can dive into this over the holidays on my own branch and then PR into this one.

@pjcozzi
Copy link
Contributor

pjcozzi commented Dec 30, 2014

Do we want this for 1.5? If so, we should aim to merge this today.

@mramato
Copy link
Contributor

mramato commented Dec 30, 2014

Definitely not for 1.5.

geometry = subTask.geometry;
if (defined(geometry.constructor.pack)) {
subTask.offset = packedLength;
packedLength += defined(geometry.constructor.packedLength) ? geometry.constructor.packedLength : geometry.packedLength;
Copy link
Contributor

Choose a reason for hiding this comment

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

I would change this to defaultValue(geometry.packedLength, geometry.constructor.packedLength);

@mramato
Copy link
Contributor

mramato commented Dec 31, 2014

I did a quick once over since I know you said you had more work you wanted to do in here. So far it looks great, this will be a significant performance improvement for loading large amounts of asynchronous geometry. Let me know when you're done and I'll give it a more complete pass.

@bagnell
Copy link
Contributor Author

bagnell commented Jan 5, 2015

@mramato This is ready for review.

@@ -731,8 +731,14 @@ define([
for (i = 0; i < length; ++i) {
geometry = instances[i].geometry;
instanceIds.push(instances[i].id);

var moduleName = geometry.constructor.name;
if (moduleName === "Object") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this to handle pre-created geometry?

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 5, 2015

Did we have performance stats for this?

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 5, 2015

This is a break change for folks implementing their own Geometry right? Can we deprecate this over one release? Some developers on the forum have implemented their own geometries.

@mramato
Copy link
Contributor

mramato commented Jan 5, 2015

@bagnell actually has to back out his last commit, which turns this into a non-breaking change. Even with the latest commit, it only changes the private API. It's currently impossible to implement an asynchronous geometry with the public api (with or without this change).

…lement packable and have a createGeometry function."

This reverts commit 4073f38.
@bagnell
Copy link
Contributor Author

bagnell commented Jan 5, 2015

Did we have performance stats for this?

The stats were posted in #2365.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 5, 2015

The stats were posted in #2365.

Thanks.

@mramato
Copy link
Contributor

mramato commented Jan 5, 2015

In Chrome, an average of 5 runs loading ne_10m_admin_0_countries.json (drag and drop wall time; me and @bagnell both have this topjson file) takes 13.05 seconds in master and 10.71 seconds in this branch, which is about an 18% speed-up. I would still like to do some more testing with various use cases.

@mramato
Copy link
Contributor

mramato commented Jan 7, 2015

I've played with this enough that I'm happy bringing it into master (plus it will have plenty of time to stew before the next release). Despite the size of the PR, it's actually not that big of a change (lots of boilerplate code for pack/unpack). I'll update CHANGES and merge.

mramato added a commit that referenced this pull request Jan 7, 2015
Pack Geometries before passing to a web worker
@mramato mramato merged commit 332bbaa into master Jan 7, 2015
@mramato mramato deleted the geometry-web-worker branch January 7, 2015 05:14
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