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

Optimize duplicate hash key warning #924

Merged

Conversation

dgollahon
Copy link
Contributor

  • Replaces O(n^2) processing loop with a set. I benchmarked parsing a file with a single 5000 key hash to test this change's performance. Before:

Before

$ hyperfine --warmup 3 'bin/ruby-parse hash_bench.rb'

Benchmark 1: bin/ruby-parse hash_bench.rb
  Time (mean ± σ):      2.678 s ±  0.029 s    [User: 2.613 s, System: 0.061 s]
  Range (min … max):    2.636 s …  2.738 s    10 runs

After

$ hyperfine --warmup 3 'bin/ruby-parse hash_bench.rb'

Benchmark 1: bin/ruby-parse hash_bench.rb
  Time (mean ± σ):     421.3 ms ±   4.1 ms    [User: 366.9 ms, System: 49.9 ms]
  Range (min … max):   414.8 ms … 426.1 ms    10 runs

- Replaces O(n^2) processing loop with a set. I benchmarked parsing a file with a single 5000 key hash to test this change's performance. Before:

**Before**

```sh
$ hyperfine --warmup 3 'bin/ruby-parse hash_bench.rb'

Benchmark 1: bin/ruby-parse hash_bench.rb
  Time (mean ± σ):      2.678 s ±  0.029 s    [User: 2.613 s, System: 0.061 s]
  Range (min … max):    2.636 s …  2.738 s    10 runs
```

**After**

```sh
$ hyperfine --warmup 3 'bin/ruby-parse hash_bench.rb'

Benchmark 1: bin/ruby-parse hash_bench.rb
  Time (mean ± σ):     421.3 ms ±   4.1 ms    [User: 366.9 ms, System: 49.9 ms]
  Range (min … max):   414.8 ms … 426.1 ms    10 runs
```

- See whitequark#918
@iliabylich iliabylich merged commit 6b1c42b into whitequark:master Apr 15, 2023
@iliabylich
Copy link
Collaborator

Thanks!

Let me know if you need a release.

@dgollahon dgollahon deleted the optimize-hash-duplicate-detection branch April 15, 2023 21:48
@dgollahon
Copy link
Contributor Author

Thanks for the fast review/merge!

Let me know if you need a release.

I'm not personally in any rush--I just happened to be following the related issue and it seemed like a fun, targeted fix.

@prikha
Copy link

prikha commented Apr 24, 2023

Awesome effort @dgollahon!

@iliabylich it'd be awesome to have a release, not urgent but some day.

@iliabylich
Copy link
Collaborator

@prikha I've released 3.2.2.1

glebm added a commit to glebm/i18n-tasks that referenced this pull request Apr 25, 2023
This is the first version which includes the fix (whitequark/parser#924) to a performance issue (whitequark/parser#918) first introduced in 3.0.3.0 (whitequark/parser@547d731)

Fixes #407
glebm added a commit to glebm/i18n-tasks that referenced this pull request Apr 25, 2023
This is the first version which includes the fix (whitequark/parser#924) to a performance issue (whitequark/parser#918) first introduced in 3.0.3.0 (whitequark/parser@547d731)

Fixes #407
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