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

Clean up and modernize code for dedupe #354

Closed
wants to merge 1 commit into from
Closed

Conversation

jonkoops
Copy link
Collaborator

@jonkoops jonkoops commented Jan 1, 2024

Cleans up the code for dedupe so that it better matches the structure of the other variants. Also introduces Set to de-duplicate classes. Benchmarks seem to indicate it's slightly faster, especially for mixed test cases:

Node.js (v21.5.0)

Benchmarking 'strings'.

Task Name ops/sec Average Time (ns) Margin Samples
dedupe/local 4,045,085 247.2135826039219 ±0.56% 2022543
dedupe/npm 4,042,492 247.37213314357965 ±0.52% 2021247

Benchmarking 'object'.

Task Name ops/sec Average Time (ns) Margin Samples
dedupe/local 7,221,118 138.48269326646596 ±0.59% 3610560
dedupe/npm 6,935,808 144.1793010477904 ±0.29% 3467905

Benchmarking 'strings, object'.

Task Name ops/sec Average Time (ns) Margin Samples
dedupe/local 4,415,599 226.46980115941534 ±0.21% 2207800
dedupe/npm 4,096,954 244.08371678890256 ±0.74% 2048478

Benchmarking 'mix'.

Task Name ops/sec Average Time (ns) Margin Samples
dedupe/local 3,083,596 324.2966677236706 ±0.74% 1541799
dedupe/npm 1,935,753 516.594635475142 ±0.76% 967877

Benchmarking 'arrays'.

Task Name ops/sec Average Time (ns) Margin Samples
dedupe/local 1,921,840 520.3345623628504 ±0.39% 960921
dedupe/npm 1,888,877 529.4150696866383 ±0.95% 944439

@dcousens
Copy link
Collaborator

dcousens commented Jan 2, 2024

Concept LGTM, I'll need to review later, happy if you want to merge and I'll come back to this 💛

@jonkoops
Copy link
Collaborator Author

jonkoops commented Jan 2, 2024

I need to give this a little more testing, I am seeing some regressions in Firefox and Chrome, not entirely sure why so I'll have to do some profiling.

@jonkoops
Copy link
Collaborator Author

jonkoops commented Jan 8, 2024

Closing this because I am seeing some inconsistencies in performance. I'll refactor the existing code in a structure that is easier to work on and then refactor it piece by piece.

@jonkoops jonkoops closed this Jan 8, 2024
@jonkoops jonkoops deleted the modernize-dedupe branch January 8, 2024 19:07
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