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

Fix destroyed fork backpressure. #411

Merged
merged 1 commit into from
Nov 28, 2015

Conversation

vqvu
Copy link
Collaborator

@vqvu vqvu commented Nov 28, 2015

See #179 (comment) for description.

@vqvu vqvu mentioned this pull request Nov 28, 2015
34 tasks
@vqvu vqvu added this to the 3.0.0 milestone Nov 28, 2015
* that only works for integer keys.
*/
function IntMap() {
this.map = {};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure if there's much point but you could reuse isES5 in index.js to see whether you could use Object.create(null) here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't do it because I'm using Object.prototype.hasOwnProperty. Is there a perf improvement to doing Object.create(null)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The perf gain is in not having to do the hasOwnProperty check but tbh like most micro-optimisations I doubt it's worth the hassle.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, gotcha. Agreed that it's not worth the hassle. Modern browsers and Nodes will just end up using global.Map anyway.

@svozza
Copy link
Collaborator

svozza commented Nov 28, 2015

Very nice implementation.

svozza added a commit that referenced this pull request Nov 28, 2015
@svozza svozza merged commit b6e50e0 into caolan:3.0.0 Nov 28, 2015
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.

2 participants