Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2691163002:
Added external memory counting for IDBValue (Closed)
DescriptionIndexedDB: Added external memory counting for IDBValue.
Should also fix https://code.google.com/p/chrome-os-partner/issues/detail?id=62731
R=cmumford@chromium.org
BUG=691607, 690517
Review-Url: https://codereview.chromium.org/2691163002
Cr-Commit-Position: refs/heads/master@{#450265}
Committed: https://chromium.googlesource.com/chromium/src/+/e7d64d6da7a8f523925a5ed80c3aea74859516e7
Patch Set 1 #Patch Set 2 : Cleaned up #
Total comments: 3
Patch Set 3 : comments #
Total comments: 1
Patch Set 4 : fixed signed int #
Total comments: 2
Messages
Total messages: 37 (21 generated)
The CQ bit was checked by dmurph@chromium.org to run a CQ dry run
dmurph@chromium.org changed reviewers: + cmumford@chromium.org
Description was changed from ========== Added external memory counting for IDBValue BUG=691607 ========== to ========== Added external memory counting for IDBValue R=cmumford@chromium.org BUG=691607 ==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Added external memory counting for IDBValue R=cmumford@chromium.org BUG=691607 ========== to ========== Added external memory counting for IDBValue R=cmumford@chromium.org BUG=691607, 62731, 690517 ==========
https://codereview.chromium.org/2691163002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/indexeddb/IDBValue.cpp (right): https://codereview.chromium.org/2691163002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/indexeddb/IDBValue.cpp:61: if (m_data && m_externalAllocatedSize) { How about instead having this: int64_t m_externalAllocatedSize = 0; in the header, so then you can just v8::Isolate::GetCurrent()->AdjustAmountOfExternalAllocatedMemory(-m_externalAllocatedSize)
https://codereview.chromium.org/2691163002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/indexeddb/IDBValue.cpp (right): https://codereview.chromium.org/2691163002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/indexeddb/IDBValue.cpp:61: if (m_data && m_externalAllocatedSize) { On 2017/02/14 00:59:24, cmumford wrote: > How about instead having this: > > int64_t m_externalAllocatedSize = 0; > > in the header, so then you can just > > v8::Isolate::GetCurrent()->AdjustAmountOfExternalAllocatedMemory(-m_externalAllocatedSize) I like it - I'm still going to check to see if it's not 0, so we don't execute the v8 logic if we don't have to.
The CQ bit was checked by dmurph@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
jsbell@chromium.org changed reviewers: + jsbell@chromium.org
https://codereview.chromium.org/2691163002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/indexeddb/IDBValue.cpp (right): https://codereview.chromium.org/2691163002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/indexeddb/IDBValue.cpp:90: return SerializedScriptValue::create(m_data->data(), m_data->size()); Note that this does a data copy (because it does byte swap, for horrible historical reasons). So we'll end up doubling memory between the IDBValue's SharedBuffer and the SerializedScriptValue's internal String. And when the SSV is deserialized to V8 we've added yet another copy to the v8 heap. Wheee. That's not new, but as a follow-on we should look at de-duping. It should (in theory...) be the case that once we've deserialized an IDBValue we should never need to deserialize it again. That's going to be hidden behind the IDBAny -> v8::Value conversion implicit in IDBRequest::result(). Similarly, once we've deserialized a SerializedScriptValue (in this case, at least) we should be able to discard the SSV.
lgtm
On 2017/02/14 01:11:35, jsbell wrote: > https://codereview.chromium.org/2691163002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/modules/indexeddb/IDBValue.cpp (right): > > https://codereview.chromium.org/2691163002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/indexeddb/IDBValue.cpp:90: return > SerializedScriptValue::create(m_data->data(), m_data->size()); > Note that this does a data copy (because it does byte swap, for horrible > historical reasons). So we'll end up doubling memory between the IDBValue's > SharedBuffer and the SerializedScriptValue's internal String. And when the SSV > is deserialized to V8 we've added yet another copy to the v8 heap. Wheee. > > That's not new, but as a follow-on we should look at de-duping. > > It should (in theory...) be the case that once we've deserialized an IDBValue we > should never need to deserialize it again. That's going to be hidden behind the > IDBAny -> v8::Value conversion implicit in IDBRequest::result(). Similarly, once > we've deserialized a SerializedScriptValue (in this case, at least) we should be > able to discard the SSV. I wonder if we can store a Persistant<Value> in the IDBValue after we do the deserialization once.
The CQ bit was unchecked by dmurph@chromium.org
The CQ bit was checked by dmurph@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dmurph@chromium.org changed reviewers: + esprehn@chromium.org
+esprehn for third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
https://codereview.chromium.org/2691163002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/indexeddb/IDBValue.cpp (right): https://codereview.chromium.org/2691163002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/indexeddb/IDBValue.cpp:63: static_cast<int64_t>(-m_externalAllocatedSize)); Doh! this is unsigned - missed that.
lgtm
haraken@chromium.org changed reviewers: + haraken@chromium.org
LGTM with a comment. https://codereview.chromium.org/2691163002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/indexeddb/IDBValue.cpp (right): https://codereview.chromium.org/2691163002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/indexeddb/IDBValue.cpp:23: v8::Isolate::GetCurrent()->AdjustAmountOfExternalAllocatedMemory( We want to deprecate v8::Isolate::GetCurrent(). Would it be possible to pass in the isolate as a parameter to IDBValue's constructor? https://codereview.chromium.org/2691163002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/indexeddb/IDBValue.cpp:62: v8::Isolate::GetCurrent()->AdjustAmountOfExternalAllocatedMemory( Ditto. We can add IDBValue::m_isolate and use it here.
Description was changed from ========== Added external memory counting for IDBValue R=cmumford@chromium.org BUG=691607, 62731, 690517 ========== to ========== IndexedDB: Added external memory counting for IDBValue. Should also fix https://code.google.com/p/chrome-os-partner/issues/detail?id=62731 R=cmumford@chromium.org BUG=691607, 690517 ==========
If there are no objections I'd like to land this as-is, being that it's a p0 release blocker, so that it can be cherry-picked into M56. We will follow-up with a second commit to do as haraken@ requests.
The CQ bit was checked by cmumford@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by cmumford@chromium.org
The CQ bit was checked by cmumford@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from cmumford@chromium.org Link to the patchset: https://codereview.chromium.org/2691163002/#ps60001 (title: "fixed signed int")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/02/14 02:53:50, cmumford wrote: > If there are no objections I'd like to land this as-is, being that it's a p0 > release blocker, so that it can be cherry-picked into M56. We will follow-up > with a second commit to do as haraken@ requests. Sure, a follow-up CL is fine :)
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487041592855510,
"parent_rev": "14d0f71b8deb5b63f498c92ccc37a65b46536e84", "commit_rev":
"e7d64d6da7a8f523925a5ed80c3aea74859516e7"}
Message was sent while issue was closed.
Description was changed from ========== IndexedDB: Added external memory counting for IDBValue. Should also fix https://code.google.com/p/chrome-os-partner/issues/detail?id=62731 R=cmumford@chromium.org BUG=691607, 690517 ========== to ========== IndexedDB: Added external memory counting for IDBValue. Should also fix https://code.google.com/p/chrome-os-partner/issues/detail?id=62731 R=cmumford@chromium.org BUG=691607, 690517 Review-Url: https://codereview.chromium.org/2691163002 Cr-Commit-Position: refs/heads/master@{#450265} Committed: https://chromium.googlesource.com/chromium/src/+/e7d64d6da7a8f523925a5ed80c3a... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/e7d64d6da7a8f523925a5ed80c3a... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
