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

Use sets instead of arrays #48

Merged
merged 2 commits into from
Feb 11, 2016
Merged

Use sets instead of arrays #48

merged 2 commits into from
Feb 11, 2016

Conversation

tbranyen
Copy link
Owner

This greatly improves the object pooling performance. No longer do we need to worry about checking indexOf on giant arrays.

@tbranyen
Copy link
Owner Author

Ping @jmeas no more array.length - 1 :D

@jamesplease
Copy link

Still reviewing the code...one thought is that you'll want to update the documentation somewhere to let people know that they'll need to include the Babel polyfill for Sets to work in all env's, unless diffHTML is limited to only the latest-and-greatest browsers?

@tbranyen
Copy link
Owner Author

True, but we also use WeakMap which isn't everywhere either. Since we're aren't going to be able to easily polyfill into Web Worker environments, it might make sense to just limit our scope to evergreen browsers.

var reAlloc = [];
cache.allocated.forEach(function (v) {
return reAlloc.push(v);
});

Choose a reason for hiding this comment

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

A succinct way to rewrite this would be:

var reAlloc = [...cache.allocated];

This compiles to [].concat() though, which creates a dynamically sized array. I wonder how that compares speed-wise to the push system you've got going. In any event, I'd expect that fixing the size would be faster than both, but I'd be curious to see the perf diff.

var reAlloc = new Array(cache.allocated.length);
cache.allocated.forEach((v, i) => reAlloc[i] = v);

Wait nvm cache.allocated is now a Set, so the first alternative wouldn't work. The second one (with the fixed-length array) should, tho' if ya swap length for size in the snippet

Copy link
Owner Author

Choose a reason for hiding this comment

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

I tried using this before, but babel compiles down to Array.from which is not available in PhantomJS. I polyfilled it there, but then it was not available in Web Workers, which seems like a huge pain to try and polyfill.

Choose a reason for hiding this comment

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

Ah, okay. Well dang. ha.

tbranyen added a commit that referenced this pull request Feb 11, 2016
@tbranyen tbranyen merged commit 1cea4c9 into master Feb 11, 2016
@tbranyen tbranyen deleted the use-sets-instead-of-arrays branch February 11, 2016 19:09
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

2 participants