Bug 105572 – IndexedDB: Use non-const buffers in put() to avoid copies

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alec Flett]

IndexedDB: Use non-const buffers in put() to avoid copies

[Alec Flett]

Created attachment 180401 [details]
Patch

[Alec Flett]

jsbell@ - quick lgtm - this is the const-ness issue we talked about this morning. This will allow async operations to hold onto the buffer while letting the caller return.

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Joshua Bell]

Does LGTM and I don't have any better suggestions. (Perhaps others will.)

[Alec Flett]

dglazgov@ - r? 

The issue here is that the implementation of put() on the other side of these interfaces wants to hold onto the value for use at a later time. i.e. the buffer inside of 'value' needs to outlive the call to put() in the first place. So we need the incoming 'value' to be non-const so that the implementation can adopt value's buffer rather than copying it.

(Also welcome to other suggestions here...)

[Alec Flett]

ugh my fingers can't spell - make that dglazkov@ - r?

[Darin Fisher (:fishd, Google)]

This may just be a personal thing, but when giving someone a reference to memory to hold on to for some time, I find pointers to be helpful from a readability point of view.  In WebKit, I'm only really accustomed to seeing non-const references used for in/out or out parameters.  It may also be helpful to document the lifetime expectations somehow.

[Alec Flett]

Created attachment 181043 [details]
Patch

[Alec Flett]

fishd@ - good point. Switched to Vector<uint8_t>*

abarth@ - r? (fishd and dglazkov are unavailable but this is pretty straightforward)

[Adam Barth]

Comment on attachment 181043 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=181043&action=review

> Source/WebKit/chromium/src/WebIDBDatabaseImpl.cpp:169
> +    Vector<uint8_t> valueBuffer(value->size());
> +    valueBuffer.append(value->data(), value->size());
> +    m_databaseBackend->put(transactionId, objectStoreId, &valueBuffer, key, static_cast<IDBDatabaseBackendInterface::PutMode>(putMode), IDBCallbacksProxy::create(adoptPtr(callbacks)), indexIds, indexKeys);

OK.  Obviously the backend can't hold on to it for too long given that it's allocated on the stack here.

[Peter Beverloo (cr-android ews)]

Comment on attachment 181043 [details]
Patch

Attachment 181043 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/15635519

[Alec Flett]

Created attachment 181083 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 181083 [details]
Patch for landing

Clearing flags on attachment: 181083

Committed r138666: <http://trac.webkit.org/changeset/138666>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.