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

[FIXED] Make access to mb's first and last sequence to consistently use atomics. #4744

Merged
merged 1 commit into from
Nov 5, 2023

Conversation

derekcollison
Copy link
Member

Fixes data races.

Signed-off-by: Derek Collison [email protected]

@derekcollison derekcollison requested a review from a team as a code owner November 5, 2023 03:37
Copy link
Member

@neilalexander neilalexander left a comment

Choose a reason for hiding this comment

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

In principle LGTM, but wonder if changing first.seq and last.seq to an atomic.Uint64 type might be less error-prone than potentially forgetting atomic.LoadUint64/atomic.StoreUint64 somewhere in the future and may also avoid any potential alignment issues?

@derekcollison
Copy link
Member Author

Might be, could not remember when they introduced those but looking now is G0 1.19, so we should be good if we want to switch. Eventually I want the top level block selection to not need atomics or locks, but more for 2.11.

What alignment issues to you forsee?

@derekcollison
Copy link
Member Author

I think we are ok alignment wise, but understand what you are saying.

I think for now will leave but agree if we keep this direction we should consider. But I think there is a better way for the upper layer indexing and block selection.

Copy link
Member

@neilalexander neilalexander left a comment

Choose a reason for hiding this comment

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

Yep agreed, LGTM!

Copy link
Member

@wallyqs wallyqs left a comment

Choose a reason for hiding this comment

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

LGTM

@derekcollison derekcollison merged commit 0b5d9c6 into main Nov 5, 2023
4 checks passed
@derekcollison derekcollison deleted the data-races branch November 5, 2023 18:34
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