Skip to content

Conversation

slavchev
Copy link

related to #65

@ns-bot
Copy link

ns-bot commented Feb 24, 2016

💚

@slavchev
Copy link
Author

ping @atanasovg @blagoev @Plamen5kov

@Plamen5kov
Copy link
Contributor

looks good 👍

}

directBufferFactory = new DirectBufferFactory(logger);
directBufferFactory.startListener();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make this initialize upon demand? I mean why starting a new thread if we are not going to use it?

@atanasovg
Copy link
Contributor

👍

@ns-bot
Copy link

ns-bot commented Feb 25, 2016

💚

@slavchev
Copy link
Author

I am missing here why we require an instance of MappedByteBuffer?

java.nio.ByteBuffer does not guarantee it will use direct buffer for its implementation while for java.nio.MappedByteBuffer it is guaranteed.

It seems that we don't cover many scenarios for TypedArray buffers exposed this way. For example creating additional views on top of already created TypeArray seems dangerous, but is not guarded or handled now.

I am not sure I understand your concerns. JavaScript typed arrays are designed to use shared ArrayBuffer. They just represent different view for the underlying ArrayBuffer.

Also TypedArrays ctor taking object arg is probably easy to support.

I am not sure this is supported by V8. Also, this will introduce unusual way for creating typed arrays. JavaScript provides a standard mechanism so I prefer to keep it that way.

Also converting an existing TypeArray to support native transfer is not easy. Maybe our arg converters can handle this.

This is another feature. This PR is focused on exposing Java memory to V8.

@blagoev
Copy link
Contributor

blagoev commented Mar 1, 2016

java.nio.ByteBuffer does not guarantee it will use direct buffer for its implementation while for java.nio.MappedByteBuffer it is guaranteed.

from SDK: "MappedByteBuffer is a special kind of direct ByteBuffer which maps a region of file to memory."
It is a direct buffer but not the only one. My suggestion is to use byteBuffer.isDirect() to validate that the passed ByteBuffer instance is a direct byte buffer and to allow any instance of ByteBuffer not just MappedByteBuffers

I am not sure I understand your concerns. JavaScript typed arrays are designed to use shared ArrayBuffer. They just represent different view for the underlying ArrayBuffer.

If I create a view of already existing ArrayBuffer it is not associated with the backing ByteBuffer and the memory will be free'd at some point under my feet while I am still using my view. Or maybe I am wrong here if v8 is keeping the original reference alive.

I am not sure this is supported by V8. Also, this will introduce unusual way for creating typed arrays. JavaScript provides a standard mechanism so I prefer to keep it that way.

Talking about the third ctor overload with parameter "object" https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/TypedArray
We don't work correctly with it. And it's part of the standard IMO.

This is another feature. This PR is focused on exposing Java memory to V8.

I think we should handle both in this implementation. Also currently this PR allows only buffers created using our own method "public ByteBuffer create(int capacity)" to work correctly so this is useful only to specific native libraries which know and call this method to allocate buffers.This handicaps the offering. I think we can provide more generic approach and work with any library that provides direct ByteBuffers to JS code.

@ns-bot
Copy link

ns-bot commented Mar 10, 2016

💔

@ns-bot
Copy link

ns-bot commented Mar 10, 2016

💔

@ns-bot
Copy link

ns-bot commented Mar 10, 2016

💚

@slavchev
Copy link
Author

Create a simplified version #379. Closing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants