Skip to content
This repository has been archived by the owner on Oct 21, 2022. It is now read-only.

More efficient DefaultDict iteration #121

Merged
merged 1 commit into from
Oct 1, 2022

Conversation

BioTurboNick
Copy link
Member

@BioTurboNick BioTurboNick commented Oct 1, 2022

The current DefaultDict iteration is extremely expensive, mostly due to reliance on Iterators.flatten. This change cuts many of these excessive allocations. See: JuliaPlots/Plots.jl#4237 (comment)

Although it would be even better if we didn't have to preserve the order. Do we? collect(union(x, y)) is a bit more memory efficient, at the cost of lost order. But the current PR preserves exactly what the current code does.

@codecov-commenter
Copy link

codecov-commenter commented Oct 1, 2022

Codecov Report

Base: 62.15% // Head: 61.75% // Decreases project coverage by -0.39% ⚠️

Coverage data is based on head (6a982ac) compared to base (6398baa).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #121      +/-   ##
==========================================
- Coverage   62.15%   61.75%   -0.40%     
==========================================
  Files          10       10              
  Lines         539      536       -3     
==========================================
- Hits          335      331       -4     
- Misses        204      205       +1     
Impacted Files Coverage Δ
src/utils.jl 59.15% <100.00%> (-1.12%) ⬇️
src/api.jl 43.18% <0.00%> (-2.28%) ⬇️
src/RecipesPipeline.jl 100.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@t-bltg t-bltg left a comment

Choose a reason for hiding this comment

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

union! explicitly states the preservation of ordering between arguments, so LGTM.

Thanks !

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants