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

Make result order deterministic #126

Merged
merged 8 commits into from
Jan 19, 2024
Merged

Conversation

camdencheek
Copy link
Member

@camdencheek camdencheek commented Dec 26, 2023

This PR makes the order of results in a Result.*Pool deterministic so that the order of the result slice corresponds with the order of tasks submitted. As an example of why this would be useful, it makes it easy to rewrite iter.Map in terms of ResultPool. Additionally, it's a generally nice and intuitive property to be able to match the index of the result slice with the index of the input slice.

Comments inline.

@codecov-commenter
Copy link

codecov-commenter commented Dec 26, 2023

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (30a99cd) 99.31% compared to head (e1e9af2) 98.94%.

Files Patch % Lines
pool/result_pool.go 94.73% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #126      +/-   ##
==========================================
- Coverage   99.31%   98.94%   -0.38%     
==========================================
  Files          12       12              
  Lines         441      474      +33     
==========================================
+ Hits          438      469      +31     
- Misses          3        4       +1     
- Partials        0        1       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@camdencheek camdencheek changed the title Draft: make result order deterministic Make result order deterministic Dec 26, 2023
Comment on lines 85 to 90
type resultAggregator[T any] struct {
mu sync.Mutex
len int
results []T
errored []int
}
Copy link
Member Author

Choose a reason for hiding this comment

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

The changes to resultAggregator are central to this PR.

Summary:

  • Add a nextIndex method that makes it possible to reserve a slot in the result slice
  • Change the add method to save, which also takes an the index from nextIndex and whether the result errored
  • Add a collect method that respects the WithCollectErrored option by filtering out errored results.

Copy link
Member

@bobheadxi bobheadxi left a comment

Choose a reason for hiding this comment

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

Nice, LGTM - this does add an extra set of lock/unlock on the mutex, though I imagine it doesn't make much difference in practice

@camdencheek
Copy link
Member Author

this does add an extra set of lock/unlock on the mutex

Yes, it does. It would be possible to use an atomic integer for reserving a slot, but it adds more complexity of the implementation and I wouldn't expect these to be a high-contention since in most cases, the task being done in a goroutine is large. We can do some benchmarking and improve perf later if the difference turns out to be significant.

@camdencheek camdencheek merged commit 8427ccd into main Jan 19, 2024
2 checks passed
@camdencheek camdencheek deleted the cc/deterministic-result-order branch January 19, 2024 17:50
@sagikazarmark
Copy link

Any plans to tag a new release with this change?

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.

4 participants