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

Implement lock-free fast_random function #596

Merged
merged 5 commits into from
Oct 7, 2023

Conversation

ForeverASilver
Copy link
Contributor

Commit for #426

  • nrand48() is replaced with splitmix32()
  • obtaining the seed is now an atomic fetch and add
  • replaced pthread_mutex_lock and unlock with single pthread_once() call to an state initialization function for the seed

@gavv gavv added ready for review Pull request can be reviewed contribution A pull-request by someone else except maintainers labels Oct 6, 2023
Copy link
Member

@gavv gavv left a comment

Choose a reason for hiding this comment

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

Thanks for PR! Here are a few comments.

@gavv gavv added needs revision Pull request should be revised by its author and removed ready for review Pull request can be reviewed labels Oct 6, 2023
@gavv
Copy link
Member

gavv commented Oct 6, 2023

Not sure if it's ready for review, but I checked new commits, the new version looks good!

I think this CI job failed because you have different version of clang-format. https://github.com/roc-streaming/roc-toolkit/actions/runs/6436815832/job/17480913363?pr=596

Which one do you use? I'm using 14.0.6. I think you can rollback changes in hashmap.h and check-formatting job will pass.

Other CI jobs failed because fast_random() is missing doxygen comment, please add it.

Please ping me when CI is fixed and PR is ready for review.

@ForeverASilver
Copy link
Contributor Author

Weirdly enough formatting is still somehow an issue and i also have clang-format 14.0.6

@gavv
Copy link
Member

gavv commented Oct 7, 2023

I've pushed commit to this PR's branch that rolls back formatting: 671b26d

Here is how you can run the same version of clang-format that is used on CI:

docker run -t --rm -u "${UID}" -v "${PWD}:${PWD}" -w "${PWD}" rocstreaming/env-ubuntu scons -Q fmt

it should produce the same results that are expected on CI.

Ideally we need to investigate what's the problem and maybe adjust hashmap code to be unaffected by it, but we have ongoing issue that also touches hashmap (#579) and I'd like to avoid creating conflicts.

So for now, if you will work on something else in roc, you can use the docker command above. We can return to this problem later if needed.


Thanks for the patch! I'll merge it after CI passes.

@gavv gavv merged commit 2bcf831 into roc-streaming:develop Oct 7, 2023
31 checks passed
@gavv gavv removed the needs revision Pull request should be revised by its author label Oct 7, 2023
@gavv gavv added this to the 0.3.0 milestone Nov 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution A pull-request by someone else except maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants