-
Notifications
You must be signed in to change notification settings - Fork 429
Add Javadoc usage examples to RingBuffer #342
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
base: master
Are you sure you want to change the base?
Conversation
|
Thank you for the update. I'll provide review comments shortly. |
agrona/src/main/java/org/agrona/concurrent/ringbuffer/ManyToOneRingBuffer.java
Outdated
Show resolved
Hide resolved
agrona/src/main/java/org/agrona/concurrent/ringbuffer/RingBuffer.java
Outdated
Show resolved
Hide resolved
agrona/src/main/java/org/agrona/concurrent/ringbuffer/RingBuffer.java
Outdated
Show resolved
Hide resolved
863f04d to
2029e2d
Compare
|
@pveentjer Your feedback is really valuable and full of great points for me to learn from. I appreciate the detailed explanations — they helped me better understand the design intent behind these classes and improve the docs accordingly |
2029e2d to
fdfa5bd
Compare
| * | ||
| * // Write a simple message | ||
| * int length = 8; | ||
| * MutableDirectBuffer srcBuffer = new UnsafeBuffer(ByteBuffer.allocateDirect(length)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code gives the impression it is fine to create ad hoc MutableDirectBuffers. In low latency systems, you don't want to create litter on the fast path because it creates a lot of performance problems. From increased mean to increased latency tail.
It should be clear to the end user that a cached buffer should be used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pveentjer
In recent chagnes, makes it clear that the buffer should be created once and reused for multiple writes, which is the main point of your review.
fdfa5bd to
be1b46e
Compare
| * RingBuffer ringBuffer = new OneToOneRingBuffer(new UnsafeBuffer(byteBuffer)); | ||
| * | ||
| * // Pre-allocate a reusable buffer (cached for the lifetime of the writer) | ||
| * MutableDirectBuffer srcBuffer = new UnsafeBuffer(ByteBuffer.allocateDirect(64)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm nit picking; but could you make the buffer 8 bytes since that is the size of a long. Use Long.BYTES as the constant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
God is in the details. good point. i will change docs and back to you again
| * // Write a simple message using the cached buffer | ||
| * int length = 8; | ||
| * srcBuffer.putLong(0, 12345L); | ||
| * ringBuffer.write(1, srcBuffer, 0, length); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are ignoring the return value of the write. It could be that the write isn't accepted because there insufficient space. If you don't handle the return value properly, messages can appear to get lost.
| * ringBuffer.read((msgTypeId, buffer, index, msgLength) -> { | ||
| * long value = buffer.getLong(index); | ||
| * System.out.println("Received: " + value); | ||
| * }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also add an example using the tryClaim. The advantage of tryClaim above offer is that you can directly write to the memory of the ringbuffer instead of writing to an intermediate buffer first.
be1b46e to
1841ff5
Compare
|
@pveentjer Hello again, new chagne focus on:
|
1841ff5 to
fd5e325
Compare
| * System.err.println("Failed to claim space: insufficient capacity"); | ||
| * } | ||
| * | ||
| * // Read the message |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the read example is a copy, I would drop it.
| * buffer.putLong(index, 12345L); | ||
| * ringBuffer.commit(index); | ||
| * } | ||
| * catch (final Exception ex) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm scratching my head on this one. Typically you never want to run into an exception on the fast path because they are very slow (e.g. tons of litter). So I'm not sure if guarding the tryClaim should be guarded in this manner. I'm going to ask and will get back to you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The javadoc of the tryClaim is very clear about it. And there should indeed by a try/catch block.
|
Thanks for your efforts on this PR. It is heading in the right direction. |
Appreciate the feedback — I’ll push the next improvements shortly. |
ea42b09 to
f0a0e20
Compare
|
@pveentjer latest change is:
|
| * | ||
| * // Write a message using the cached buffer | ||
| * srcBuffer.putLong(0, 12345L); | ||
| * if (!ringBuffer.write(1, srcBuffer, 0, Long.BYTES)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that the tryClaim already has usage documentation. So having it in the header is just duplication.
Could you move the documentation of the read and write to the method level instead of the class level so it is consistent with the tryClaim.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point, It's done. I just move it and in class level read and write. also some other change, if it's not good please let me know to rellong back this changes
f751e76 to
0cafc4f
Compare
0cafc4f to
58df786
Compare
| * in a FIFO manner. | ||
| * <p> | ||
| * Supports non-blocking writes via {@link #write(int, DirectBuffer, int, int)} and | ||
| * zero-copy writes via {@link #tryClaim(int, int)}. Messages are read by consumers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I would be a noob, I could interpret this as: I can do non blocking writes using write(...), but tryClaim is blocking but zero copy?
In other words, the way this is phrased, can cause confusion.
Hi @pveentjer,
Thanks for your previous review and helpful feedback
I’ve considered your comments and moved the example usage to the appropriate classes.
This new pull request updates the Javadoc by:
• Adding the basic usage example to MutableDirectBuffer
• Adding atomic operation examples to AtomicBuffer
Appreciate your time and guidance in the review process!