-
Notifications
You must be signed in to change notification settings - Fork 566
Add CefV8BackingStore for off-thread ArrayBuffer data population #4079
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
…ingStore() to allow creating ArrayBuffer backing memory on the renderer thread, populating it with data on a background thread (avoiding expensive memcpy on the JS thread), and then creating a zero-copy ArrayBuffer from it. This is particularly useful when V8 sandbox is enabled, where CreateArrayBuffer (external buffer) is not supported and CreateArrayBufferWithCopy performs the memcpy on the JS thread. Changes: - Add CefV8BackingStore interface with Create(), Data(), ByteLength(), IsValid() - Add CefV8Value::CreateArrayBufferFromBackingStore() static factory method - Add CefV8BackingStoreImpl with thread-safe access and move semantics - Add V8TEST_ARRAY_BUFFER_BACKING_STORE unit test
magreenblatt
left a comment
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.
Thanks for this PR. Please see comments inline.
- Add byte_length==0 check in CefV8BackingStore::Create() - Add [[nodiscard]] attribute to TakeBackingStore()
|
Thanks for the review, comments have been resolved. I was wondering if I would make getter for CefV8ArrayBuffer to retreive the underlying CefV8BackingStore also. It already has Data getter. What you think? |
| virtual void ReleaseBuffer(void* buffer) = 0; | ||
| }; | ||
|
|
||
| /// |
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.
New classes/methods need to be protected by #if CEF_API_ADDED(CEF_NEXT) with appropriate /*--cef(added=next)--*/ metadata. See https://bitbucket.org/chromiumembedded/cef/wiki/ApiVersioning.md#markdown-header-usage-in-pull-requests
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.
Thanks, fixed that
| /// Returns a pointer to the allocated memory, or nullptr if the backing | ||
| /// store has been consumed or is otherwise invalid. The pointer is safe to | ||
| /// read/write from any thread. The caller must ensure all writes are complete | ||
| /// before passing this object to CreateArrayBufferFromBackingStore(). |
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.
Do we need a warning here about not keeping a pointer reference after passing to CreateArrayBufferFromBackingStore?
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.
Added a warning to the Data() doc comment: "Pointers obtained from this method should not be retained after calling CreateArrayBufferFromBackingStore(), as the memory will then be owned by the ArrayBuffer and subject to V8 garbage collection."
- Wrap CefV8BackingStore class and CreateArrayBufferFromBackingStore with - Add warning to Data() about not retaining pointer after CreateArrayBufferFromBackingStore
|
Hey, thanks for the PR! I have a question about the As I see it, the lock doesn't fully make the class thread-safe. It protects individual method calls, but once What do you think about removing the lock and instead documenting that the class is safe to pass between threads, but not to share across threads simultaneously? This seems to match the intended usage pattern: |
Summary
CefV8BackingStoreclass that wraps V8'sArrayBuffer::BackingStore, allowing the backing memory to be allocated on the renderer thread and populated (e.g.memcpy) on a background threadCefV8Value::CreateArrayBufferFromBackingStore()static method to create an ArrayBuffer from a pre-populated backing store in a zero-copy operationCreateArrayBuffer(external buffer)Motivation
Currently with V8 sandbox enabled,
CreateArrayBufferis not supported andCreateArrayBufferWithCopyperforms thememcpyon the JS thread. For large buffers this blocks the renderer. This API enables:CefV8BackingStore::Create(size)— allocate sandbox-compatible memoryData()pointer — expensive work off JS threadCreateArrayBufferFromBackingStore(store)— zero-copy ArrayBuffer creationAPI
Test plan
cef_unittests --gtest_filter="V8Test.ArrayBufferBackingStore"— covers creation, data access, zero-copy ArrayBuffer, consumption invalidation, double-use rejection, and neutering