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

improve .unique performance #184

Closed
catalint opened this issue Apr 4, 2016 · 13 comments
Closed

improve .unique performance #184

catalint opened this issue Apr 4, 2016 · 13 comments
Labels
Milestone

Comments

@catalint
Copy link
Contributor

@catalint catalint commented Apr 4, 2016

When not sending a key , we could you the new ES6 Set feature, it delivers a ~ 2x speed performance

http://jsperf.com/array-unique-hoek-vs-es6

@DavidTPate

This comment has been minimized.

Copy link

@DavidTPate DavidTPate commented Apr 4, 2016

I'm sure they would accept a PR for it if you have a chance.

@Marsup

This comment has been minimized.

Copy link
Member

@Marsup Marsup commented Apr 5, 2016

It's a breaking change on complex objects/arrays.

@catalint

This comment has been minimized.

Copy link
Contributor Author

@catalint catalint commented Apr 5, 2016

In the pull request I've also taken care of the key param for objects and I still see a performance improvement, could you check that ?

@Marsup

This comment has been minimized.

Copy link
Member

@Marsup Marsup commented Apr 5, 2016

That's a change of behavior nonetheless, I'm not saying it's not good, it just should be a new major.

@catalint

This comment has been minimized.

Copy link
Contributor Author

@catalint catalint commented Apr 5, 2016

Yes, my thoughts also, to few tests on this

On Tue, Apr 5, 2016, 13:02 Nicolas Morel notifications@github.com wrote:

That's a change of behavior nonetheless, I'm not saying it's not good, it
just should be a new major.


You are receiving this because you authored the thread.

Reply to this email directly or view it on GitHub
#184 (comment)

@DavidTPate

This comment has been minimized.

Copy link

@DavidTPate DavidTPate commented Apr 5, 2016

Looks like this raises that old issue as well where it doesn't handle 2 different from "2" either. I know I saw an implementation in Joi that would handle those types and fix the uniqueness check (assuming it is considered a bug when it can't differentiate between types for uniqueness). That same concept could be applied here.

@catalint

This comment has been minimized.

Copy link
Contributor Author

@catalint catalint commented Apr 5, 2016

I wouldn't expect this function to do data transformations, user must be
aware of this and consolidate any input data before using it

Catalin Tanasescu

Telefon: 0722739074

On Tue, Apr 5, 2016 at 3:26 PM, David Pate notifications@github.com wrote:

Looks like this raises that old issue as well where it doesn't handle 2
different from "2" either. I know I saw an implementation in Joi
https://github.com/hapijs/joi/blob/93f914009de02c378e3dc1e0b2d6bce6cbb732a0/lib/array.js#L468-L475
that would handle those types and fix the uniqueness check (assuming it is
considered a bug when it can't differentiate between types for uniqueness).
That same concept could be applied here.


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#184 (comment)

@devinivy

This comment has been minimized.

Copy link
Member

@devinivy devinivy commented Apr 5, 2016

(Just to be explicit) I assume everyone is alluding to the fact that a Set holds typed values, whereas object keys coerce values to string. I think this is an upgrade 👍. Would any packages within the hapijs org have significant trouble dealing with a hoek major bump that includes this change? A major bump could also include removing isAbsolutePath() (#179).

@hueniverse

This comment has been minimized.

Copy link
Member

@hueniverse hueniverse commented Apr 6, 2016

Someone should just check where this is used in hapi and make sure it won't cause issues.

@devinivy

This comment has been minimized.

Copy link
Member

@devinivy devinivy commented Apr 6, 2016

hapi itself only uses unique() here, which does not seem like an issue.

@AdriVanHoudt

This comment has been minimized.

Copy link
Contributor

@AdriVanHoudt AdriVanHoudt commented Apr 6, 2016

doing grep -lr "Hoek.unique(" . on hapi dir results in:

./lib/connection.js
./node_modules/catbox/node_modules/joi/lib/string.js
./node_modules/heavy/node_modules/joi/lib/string.js
./node_modules/hoek/README.md
./node_modules/hoek/test/index.js
./node_modules/joi/lib/string.js
./node_modules/statehood/node_modules/joi/lib/string.js

also quick github search resulted only in Joi (inert and subtext use uniqueFileName)

@devinivy

This comment has been minimized.

Copy link
Member

@devinivy devinivy commented Apr 6, 2016

I can verify that joi uses unique() only on an array of strings, which is also no problem.

@nlf nlf added the request label Apr 7, 2016
@catalint

This comment has been minimized.

Copy link
Contributor Author

@catalint catalint commented Apr 25, 2016

updated pull request with more tests, let me know if I can further improve this, thanks!

@nlf nlf modified the milestone: 4.0.0 Apr 25, 2016
@nlf nlf closed this Apr 25, 2016
@Marsup Marsup added feature and removed request labels Sep 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.