rust-itertools / itertools

Extra iterator adaptors, iterator methods, free functions, and macros.
https://docs.rs/itertools/
Apache License 2.0
2.71k stars 309 forks source link

Implement `DuplicatesBy::fold` #921

Open kinto-b opened 5 months ago

kinto-b commented 5 months ago

Relates to #755. I didn't grab the output earlier (is there a way to print without re-benchmarking?) but this makes no difference to the performance. Submitting a PR for record keeping sake

{
  "mean": {
    "confidence_interval": {
      "confidence_level": 0.95,
      "lower_bound": -0.0081767782683812,
      "upper_bound": 0.0038785180687818605
    },
    "point_estimate": -0.00312222985963162,
    "standard_error": 0.003180882676320771
  },
  "median": {
    "confidence_interval": {
      "confidence_level": 0.95,
      "lower_bound": -0.009367663685134739,
      "upper_bound": -0.003824443197890548
    },
    "point_estimate": -0.006249808601540896,
    "standard_error": 0.0014046028043885176
  }
}
codecov[bot] commented 5 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 94.44%. Comparing base (6814180) to head (73d78c5). Report is 54 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #921 +/- ## ========================================== + Coverage 94.38% 94.44% +0.06% ========================================== Files 48 48 Lines 6665 6882 +217 ========================================== + Hits 6291 6500 +209 - Misses 374 382 +8 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Philippe-Cholet commented 5 months ago

Same for #918 and #919, it uses a HashMap and therefore heap allocates and (securely but slowly) hash things, those two things are the bottleneck. There is not much to expect of such specialization without improving that front:

With a faster hasher, maybe that specialize fold would a bit more beneficial for the benchmark to show some improvement. We might want to revisit those PRs once I make a more general DuplicatesByIn.

I referenced your PRs in https://github.com/rust-itertools/itertools/issues/755#issuecomment-1728319901 to avoid people from making duplicates PRs.

There would also be rfold to consider alongside fold.


Side note: I'm aware specialization benchmarks are slow to build (2 minutes for me), and 8 seconds per benchmark to run (which is ok). I usually comment out (or simply remove) benchmarks from the bench_specializations macro declaration (without committing changes) to have fast benchmarks. With that, I don't mind re-run benchmarks. It's a pain but it's a huge file with a lot of iterators and methods to bench, there is no simple fix to avoid building benchmarks we are not gonna use.