-
Notifications
You must be signed in to change notification settings - Fork 68
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
3.0.0 Switched to hashmap with performance improvements #48
base: master
Are you sure you want to change the base?
Conversation
… I included the support for NaN which never equals itself, we really shouldn't.
using << instead of ^ because its not an xor its a pow
fixing bitshifts, need to test this later
made the code functionality identical, added self to copyright as 2/3 of code is new. Performance: 2.4.0 is 38.76 X faster on _create_ an increase of 3775.95% 3.0.0 is 1.76 X faster on _singleSet_ an increase of 76.11% 3.0.0 is 2.16 X faster on _singleSet 20_ an increase of 116.21% 3.0.0 is 1.91 X faster on _singleReplace_ an increase of 90.53% 3.0.0 is 1.44 X faster on _setAfter 1,024_ an increase of 44.02% 3.0.0 is 1.91 X faster on _set 20 After 1,024_ an increase of 90.84% 3.0.0 is 3.25 X faster on _setAfter 131'072_ an increase of 225.36% 3.0.0 is 3.70 X faster on _set 20 After 131'072_ an increase of 270.20%
I appreciate your hard work and the fact that you submit it to the project, but I'll be honest with you. This project was created several years prior to the introductions and standarization of the JavaScript I'm sure your implementation is fast but I don't want to go thru merging, testing, iterating, fixing a bug here and there. |
But people are using it, and the problem is its top of the list and not doing what it claims. This new impl is faster than the basic map function, because that's what is underpinning the original. More importantly it is a hashmap, which has very specific performance guarantees, which the original is claiming but can't deliver, as it is not and never was a hasmap. I was trying to give back to the community, not create unnecessary competition. |
Of course it does what it claims, I understand you care about that but it's the first time someone complains in the 7-8 years it exists. People needed a map that indexes something other than numbers and strings and got it. |
Dude I know what I am talking about here. It is 100% not a hashmap, it is a map with strings as keys, but not a hashmap. Popularity or lack of complaints does not mean I am wrong. I am trying to help you here, get a second opinion, willing to offer £500 to your £100 if I am wrong, I could do with the cash. Also you are indexing by string, there is a promise in your notes that you don't stringify the objects, but that is exactly what you do, you are just adding card image graphemes to the front for uniqueness, which btw has holes in the approach. And you are right a map will do a better job. But a properly segregated hashmap will be faster. I think you should be honest at least in your documentation. |
Switched to a hashmap trie. With significantly faster insertion rates, creating a new instance is slower, albeit that is likely to be done substantially less than modification of the map. There is potential to improve performance, especially around create. Let me know what you think.
2.4.0 is 38.76 X faster on create an increase of 3775.95%
3.0.0 is 1.76 X faster on singleSet an increase of 76.11%
3.0.0 is 2.16 X faster on singleSet 20 an increase of 116.21%
3.0.0 is 1.91 X faster on singleReplace an increase of 90.53%
3.0.0 is 1.44 X faster on setAfter 1,024 an increase of 44.02%
3.0.0 is 1.91 X faster on set 20 After 1,024 an increase of 90.84%
3.0.0 is 3.25 X faster on setAfter 131'072 an increase of 225.36%
3.0.0 is 3.70 X faster on set 20 After 131'072 an increase of 270.20%