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

[Cpp] Added tryClaim method to concurrent ringbuffers. #1325

Conversation

im95able
Copy link

@im95able im95able commented May 24, 2022

In the codebase my team is working on we would like to use concurrent::ringbuffers. For us, tryClaim interface is preferred. I tried following you're code convetion as much as possible but reused the tryClaim implementaion for write methods(that way tryClaim also gets tested with write).
Unfortunately order of atomic writes for oneToOneRingBuffer was changed. Tail gets written first and only after does messageHeader get written. This could affect performance.
In our implementation of spsc channel we provide the callback to the tryClaim method which gets called with the claimed buffer. That way tail gets written after the messageHeader; wanting to stay within the convention of your codebase this PR does it by introducing RingBufferClaim class.

Edit: As per @mjpt777's suggestion refactored the code to reflect Java RingBuffers.

@mjpt777
Copy link
Contributor

mjpt777 commented May 31, 2022

It would be best if the implementations followed the same API semantics as we have for the Java RingBuffers.

https://github.com/real-logic/agrona/tree/master/agrona/src/main/java/org/agrona/concurrent/ringbuffer

@im95able im95able force-pushed the feature/client/concurrent/ringbuffers/try-claim branch from a3e6ade to defcab0 Compare June 2, 2022 06:47
@im95able
Copy link
Author

im95able commented Jun 8, 2022

It would be best if the implementations followed the same API semantics as we have for the Java RingBuffers.

https://github.com/real-logic/agrona/tree/master/agrona/src/main/java/org/agrona/concurrent/ringbuffer

Will do

@im95able im95able requested a review from mikeb01 June 10, 2022 06:37
@im95able
Copy link
Author

@mjpt777 could there be a review of this ?

Copy link
Contributor

@vyazelenko vyazelenko left a comment

Choose a reason for hiding this comment

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

There are two things that stand out:

  • Usage of the RecordDescriptor::makeHeader to combine length and type fields together.
    We have decided that it would be better if the C++ version was aligned with the Java and C implementations, i.e. it should read/write length and type as separate fields.
  • OneToOneRingBuffer::claimCapacity is not aligned with the Java implementation, i.e. in Java and in C we add an additional HEADER_LENGTH when computing the requiredCapacity. This allows for pre-zeroing of the next header upon claim and thus we don't need to zero the memory in the read/controlledRead methods.

Another thing to note is that the C++ and C implementations don't have the controlledRead API.

@im95able
Copy link
Author

There are two things that stand out:

  • Usage of the RecordDescriptor::makeHeader to combine length and type fields together.
    We have decided that it would be better if the C++ version was aligned with the Java and C implementations, i.e. it should read/write length and type as separate fields.
  • OneToOneRingBuffer::claimCapacity is not aligned with the Java implementation, i.e. in Java and in C we add an additional HEADER_LENGTH when computing the requiredCapacity. This allows for pre-zeroing of the next header upon claim and thus we don't need to zero the memory in the read/controlledRead methods.

Another thing to note is that the C++ and C implementations don't have the controlledRead API.

Will fix this probably some time next week. Thanks for the input.

@mjpt777 mjpt777 removed the request for review from mikeb01 August 19, 2022 14:46
@mjpt777
Copy link
Contributor

mjpt777 commented Aug 19, 2022

Is this progressing?

@mjpt777 mjpt777 closed this Jan 18, 2023
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.

None yet

4 participants