Comments (8)
It doesn't seem that the doc comments on the relevant code are changed in sync with the changes. For instance, OakUnsafeDirectBuffer
's getByteBuffer()
returns a new ByteBuffer
on every invocation of the method. But the doc comment still says that state of the returned ByteBuffer
shouldn't be modified mentioning it might be shared and used elsewhere.
from oak.
Hi @dynaxis ,
Thanks for your comment. Here is the comment/documentation about OakUnsafeDirectBuffer
's getByteBuffer()
public interface OakUnsafeDirectBuffer {
/**
* Return the underlying memory address wrapped as a ByteBuffer.
* This buffer might contain data that is unrelated to the context in which this object was introduced.
* Note 1: depending on the context (casting from OakScopedReadBuffer or OakScopedWriteBuffer), the buffer mode
* might be ready only.
* Note 2: the buffer internal state (e.g., byte order, position, limit and so on) should not be modified as this
* object might be shared and used elsewhere.
*
* @return the underlying memory address wrapped as a ByteBuffer.
*/
ByteBuffer getByteBuffer();
Indeed, a new ByteBuffer
object is returned on each invocation. However, the underlying memory referenced from this ByteBuffer
is always the same (for the same OakUnsafeDirectBuffer
object). Therefore, indeed, modifying those two ByteBuffers
may lead to corrupted results. This is why the interface is named Oak-Unsafe-DirectBuffer and it needs to be used with extra care.
How would you suggest to rephrase the documentation so it is more clear?
from oak.
Hi @sanastas,
The doc comment is a little bit misleading in the following points, IMHO:
-
This buffer might contain data that is unrelated to the context in which this object was introduced.
I think this is potentially misleading in that currently the returned
ByteBuffer
is configured to prohibit access to outside of the bounds. It should be safe to touch anywhere within the bounds, isn't it? -
Note 2: the buffer internal state (e.g., byte order, position, limit and so on) should not be modified as this object might be shared and used elsewhere.
In the similar way, it's not possible to touch outside of the configured boundaries. Also the
ByteBuffer
instance is no longer shared among multiple threads. So it should be ok to change byte order, position, limit and so on.
So my suggestion for a better wording is as follows:
public interface OakUnsafeDirectBuffer {
/**
* Returns a ByteBuffer wrapping the underlying memory segment backing this buffer.
*
* Since a new ByteBuffer instance is returned on each invocation of this method, it's absolutely safe
* to modify the state of the ByteBuffer including its byte order, position, and limit.
*
* Note that the buffer mode of the returned ByteBuffer might be read-only,
* depending on whether this buffer has been casted from OakScopedReadBuffer or OakScopedWriteBuffer.
*
* @return a ByteBuffer wrapping the underlying memory segment
*/
ByteBuffer getByteBuffer();
UPDATE: I also found that the ByteBuffer
is never read-only different from the note 2 in the current doc comment. So should we remove the last sentence starting with Note that the buffer...
or make the ByteBuffer
s returned from read only buffers actually read-only?
In my opinion, ByteBuffer
s returned should be set read-only if they are from read-only buffer.
from oak.
Hi @dynaxis ,
Thank you for your reply. Your suggestion makes sense. Although I would stress that this buffers brings you no protection from possible concurrent update of the same underlying memory from different ByteBuffer
objects. If you are working with single thread then no problem, of course.
public interface OakUnsafeDirectBuffer {
/**
* Returns a ByteBuffer wrapping the underlying memory segment backing this buffer.
* NOT THREAD SAFE! Two ByteBuffers of the same OakUnsafeDirectBuffer will reference to the same memory,
* so concurrent access with at least one write may result in inconsistent state of the data.
*
* Since a new ByteBuffer instance is returned on each invocation of this method, it's absolutely safe
* to modify the state of the ByteBuffer including its byte order, position, and limit.
*
* Note that the buffer mode of the returned ByteBuffer might be read-only,
* depending on whether this buffer has been casted from OakScopedReadBuffer or OakScopedWriteBuffer.
*
* @return a ByteBuffer wrapping the underlying memory segment
*/
ByteBuffer getByteBuffer();
As for read-only buffer, while creating the buffer inside DirectUtils
's wrapAddress
we need to check for instance of OakScopedWriteBuffer
and only then let the ByteBuffer
be writable, otherwise asReadOnlyBuffer()
.
@dynaxis , as the change is quite easy, would you be able to provide a PR, so I will review and merge to master? So you will join our small but powerful contributors community? :)
from oak.
Dear @sanastas,
I agree with your additions to the doc comment, though I might suggest a bit of rewording. Also it's good to issue a PR myself.
BTW, I think the check for read only buffer should be inside getByteBuffer()
method rather than DirectUtils.wrapAddress()
. Otherwise DirectUtils
will have unnecessary tight coupling with buffer types.
Anyway, I'll prepare a PR soon and come back.
from oak.
Thank you very much @dynaxis , do as you think is the best way! Very much appreciated!
from oak.
@sanastas #209 is issued to resolve the issues I raised above.
from oak.
@dynaxis , thank you so much! Review done. Code looks good, just one comment left!
from oak.
Related Issues (20)
- Add delete-allocate stress test
- Improve Oak test coverage
- Add more common serializers HOT 2
- Ideas for Oak new usage and improvement
- Documentation Review and Additions
- OakHash API HOT 1
- Executers instead of Threads HOT 1
- OakBuffer benchmark
- Delete-Allocate Memory Consumption Benchmark
- Bug found!
- creating git actions for maven
- Add checkstyle to the tests
- Question - Is it safe to remove(...) entries while iterating over an OakMap HOT 5
- clear() throws java.lang.UnsupportedOperationException: remove HOT 1
- oak crash on macos M1 chip
- oak map replace method throw AssertionError
- off-heap memory used by oakMap can not be released HOT 8
- Can I use one OakMapBuilder to create more than one OakMap? HOT 1
- Hi, I find a strange problem about OakMap.remove() HOT 7
- infinite loop in ThreadIndexCalculator HOT 2
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from oak.