Skip to content

Concurrency fix and efficiency improvement at low srate from jfrey-xx #89

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

Closed
wants to merge 1 commit into from

Conversation

chkothe
Copy link
Contributor

@chkothe chkothe commented Oct 30, 2020

This is a merger of #70 along with the queue fix from #71 (and some tweaks).

@tstenner
Copy link
Collaborator

tstenner commented Nov 2, 2020

So far, looking good. The locking seems fine to me and the benchmark that exhibited the crash / infinite loop runs through just fine.
Two things: there are a bunch of indentation changes that make it hard to see what changed (or who changed a line last with git blame) and the commit authorship didn't survive the rebasing.

I've split the PR into two commits (@jfrey-xx's changes and yours) and once the CI is happy I'll merge it.

@jfrey-xx
Copy link
Contributor

jfrey-xx commented Nov 2, 2020

Thanks a lot for bringing in the changes! It will for sure help a lot here :) Very minor remark, but since I noticed it: in the changelog from tstenner@340d517 I believe that the race condition is actually from #71 ^^

@tstenner
Copy link
Collaborator

tstenner commented Nov 2, 2020

Thanks a lot for bringing in the changes! It will for sure help a lot here :) Very minor remark, but since I noticed it: in the changelog from tstenner@340d517 I believe that the race condition is actually from #71 ^^

fixed

@tstenner tstenner closed this Nov 2, 2020
@chkothe
Copy link
Contributor Author

chkothe commented Nov 2, 2020

Ah sorry, my editor was still set to indent with 4 spaces instead of tabs.

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