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 performance regression on Node 4x #55

Closed
wants to merge 1 commit into from
Closed

Conversation

aheckmann
Copy link
Contributor

see #54

Note: This is a forwards compatible only change, it works on any NodeJS/iojs release with Map support. Using plain objects isn't working well on NodeJS 4.0 - 4.2.1. See #54 for details. Not sure if this is the direction you'd like to go for this module but it seems to me the alternatives are:

  1. wait for V8 to fix the bug (or fix it ourselves). In the meantime put a disclaimer on the README
  2. use a different module
  3. this PR
  4. use another fancier approach, like maybe use multiple cache objects with some manual sharding to keep < 8193 keys per cache object
  5. ?

RE: failed tests - some fail due to c++ build failure.

@isaacs
Copy link
Owner

isaacs commented Nov 27, 2015

Alright!

As it happens, it's quite straightforward to implement a simple Map polyfill. I've landed this patch, and then another to remove the restriction that keys be numbers or strings, and published as 3.0.0.

If you're using a version of Node with Map support, then it'll Just Work, and ought to perform quite a bit better. If not, then that should also be fine.

On non-Map supporting engines, I'd expect to see an ever-so-slight decrease in performance, since there are a few more objects being created, but it should be negligible. Additionally, since it appears that many people are using Objects as keys (despite the horrible bugs this creates), I'd guess that supporting non-String keys properly will be a big win.

@isaacs isaacs closed this Nov 27, 2015
@joepie91
Copy link

There does not appear to be a changelog in the repository anywhere. Could you clarify on why this caused a major version bump (ie. what part of the API has broken) - and ideally add it to the README as a changelog entry?

@isaacs
Copy link
Owner

isaacs commented Nov 28, 2015

@joepie91 You can use non-string keys now, and they won't be coerced to a string. So, using different objects (even if they stringify the same) will be different cache entries.

@joepie91
Copy link

@isaacs I see, thanks. Probably best to add that to the README as well :)

@aheckmann aheckmann deleted the map branch November 28, 2015 01:33
briangough pushed a commit to overleaf/spelling that referenced this pull request Mar 4, 2016
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