Code Monkey home page Code Monkey logo

Comments (8)

dynaxis avatar dynaxis commented on August 15, 2024

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.

sanastas avatar sanastas commented on August 15, 2024

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.

dynaxis avatar dynaxis commented on August 15, 2024

Hi @sanastas,

The doc comment is a little bit misleading in the following points, IMHO:

  1. 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?

  2. 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 ByteBuffers returned from read only buffers actually read-only?

In my opinion, ByteBuffers returned should be set read-only if they are from read-only buffer.

from oak.

sanastas avatar sanastas commented on August 15, 2024

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.

dynaxis avatar dynaxis commented on August 15, 2024

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.

sanastas avatar sanastas commented on August 15, 2024

Thank you very much @dynaxis , do as you think is the best way! Very much appreciated!

from oak.

dynaxis avatar dynaxis commented on August 15, 2024

@sanastas #209 is issued to resolve the issues I raised above.

from oak.

sanastas avatar sanastas commented on August 15, 2024

@dynaxis , thank you so much! Review done. Code looks good, just one comment left!

from oak.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo 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.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.