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

Allow passing a comparator function to the deterministic option #49

Merged
merged 7 commits into from
Aug 1, 2024

Conversation

flobernd
Copy link
Contributor

@flobernd flobernd commented Jul 30, 2024

This allows to use a custom comparator function for the deterministic sorting of object keys.

Closes #48

@BridgeAR I'm not very familiar with JS, so please feel free to suggest improvements 🙂

Copy link
Owner

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

Thank you very much, this is already looking very good!

I think it's possible to simplify it a tad, so I left a few comments. I'll merge it with those addressed.

index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@flobernd
Copy link
Contributor Author

flobernd commented Jul 30, 2024

Hi @BridgeAR, thanks for the review. Happy to use the suggested changes.

Circumventing the fast-path is a trade-off between performance and simplicity for the purpose of not having to implement the default/fallback comparator function, I guess?

Edit:
Going to fix the linter errors as well 🙂

Edit 2:
Interesting. The standard Array.sort() is actually faster on my machine. Takes roughly ~8ms for the custom comparator test compared to ~22ms when using the insertion-sort "fast-path". This is using Node v20.12.2.

flobernd and others added 6 commits July 31, 2024 09:25
Co-authored-by: Ruben Bridgewater <ruben@bridgewater.de>
Co-authored-by: Ruben Bridgewater <ruben@bridgewater.de>
Co-authored-by: Ruben Bridgewater <ruben@bridgewater.de>
Co-authored-by: Ruben Bridgewater <ruben@bridgewater.de>
Co-authored-by: Ruben Bridgewater <ruben@bridgewater.de>
@BridgeAR BridgeAR merged commit 9a988d0 into BridgeAR:main Aug 1, 2024
7 checks passed
@BridgeAR
Copy link
Owner

BridgeAR commented Aug 1, 2024

@flobernd thank you for the contribution!

The benchmark depends on the concrete input. Objects are often not that badly sorted and the performance depends on the length and the necessary steps needed to sort the input. The native implementation is faster but there's a cost to cross the JS / C++ layer and that's why it's favorable for a couple of inputs to use the JS implementation.

For the the comparator it's probably always favorable to use the native implementation as the function calls needed can be better optimized in the native implementation.

Would you mind sharing your input that you used?

@flobernd
Copy link
Contributor Author

flobernd commented Aug 1, 2024

@BridgeAR I see, that makes sense. The test data was just the simple a,b,c object from the custom comparator test that I modified to run multiple times in a loop. A rather bad way of benchmarking, I know 😝 Just wanted to get a feeling for the performance, but given the bad test methodology and the very simple input object, the result was probably far from being accurate or even representative for a real world workload.

@flobernd
Copy link
Contributor Author

flobernd commented Aug 2, 2024

@BridgeAR Would it be possible to push the 2.5.0 release to NPM? 🙂

@flobernd
Copy link
Contributor Author

Hi @BridgeAR, sorry to bother you with this again:

Would it be possible to push the 2.5.0 release to NPM? 🙂

We can as well publish a clone, if you are busy etc.

@BridgeAR
Copy link
Owner

Hey @flobernd, I am going to to publish it in a few days. I am currently not at home but my publishing token is at home.

@flobernd
Copy link
Contributor Author

Thanks a lot @BridgeAR !

@BridgeAR
Copy link
Owner

@flobernd I just published the new version

@flobernd
Copy link
Contributor Author

Thank you @BridgeAR 🙂

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.

Feature Request: Allow customizing the sort order
2 participants