Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2904603003:
[IndexedDB] Adding txn, value, and key size metrics (Closed)
Description[IndexedDB] Adding txn, value, and key size metrics, and update txn cost.
Metrics for transaction, value, and key sizes.
Transaction size cost for quota updated to include key size estimation.
BUG=723129
Review-Url: https://codereview.chromium.org/2904603003
Cr-Commit-Position: refs/heads/master@{#476089}
Committed: https://chromium.googlesource.com/chromium/src/+/4dc3da85cabab5ebae5807dbb4ecb3d270e9072b
Patch Set 1 #
Total comments: 8
Patch Set 2 : Added blob metrics, comments, and fix for kb #Patch Set 3 : added histograms, fixed kb #
Total comments: 4
Patch Set 4 : comments and compile #Patch Set 5 : updated fakes and size interface to let tests pass #Patch Set 6 : fixed tests, ran tests #Patch Set 7 : doing math in test #Patch Set 8 : rebase #Patch Set 9 : build fix #Patch Set 10 : rebase again #Patch Set 11 : fix for error cases #Patch Set 12 : made sure size is 0'd out #Messages
Total messages: 77 (56 generated)
Description was changed from ========== [IndexedDB] Adding txn, value, and key size metrics BUG=723129 ========== to ========== [IndexedDB] Adding txn, value, and key size metrics, and update txn cost. Metrics for transaction, value, and key sizes. Transaction size cost for quota updated to include key size estimation. BUG=723129 ==========
dmurph@chromium.org changed reviewers: + pwnall@chromium.org
Victor, can you PTAL?
This is really good stuff! Thanks for putting together the CL! https://codereview.chromium.org/2904603003/diff/1/content/browser/indexed_db/... File content/browser/indexed_db/indexed_db_transaction.cc (right): https://codereview.chromium.org/2904603003/diff/1/content/browser/indexed_db/... content/browser/indexed_db/indexed_db_transaction.cc:357: UMA_HISTOGRAM_MEMORY_KB("WebCore.IndexedDB.TransactionSize", Is this number different because of the blob journal changes in CommitPhaseTwo? Can you please add a comment explaining? https://codereview.chromium.org/2904603003/diff/1/content/browser/indexed_db/... File content/browser/indexed_db/leveldb/leveldb_transaction.cc (right): https://codereview.chromium.org/2904603003/diff/1/content/browser/indexed_db/... content/browser/indexed_db/leveldb/leveldb_transaction.cc:57: size_ += sizeof(Record) + key.size() * 2 + value->size(); This book-keeping looks subtle enough to require tests. Can you add some quick unit tests today? If not, it seems fine to land this now and potentially fix later. FWIW, I was about to suggest the following plan, and my idea fell flat on its face because LevelDB doesn't report WriteBatch size. 1) Forward the WriteBatch size getter in LevelDBWriteBatch -- https://cs.chromium.org/chromium/src/content/browser/indexed_db/leveldb/level... 2) Change LevelDBTransaction::Commit to return the WriteBatch size in an out-parameter -- https://cs.chromium.org/chromium/src/content/browser/indexed_db/leveldb/level... 3) Log the value in IndexedDBBackingStore::Transaction::CommitPhaseTwo --https://cs.chromium.org/chromium/src/content/browser/indexed_db/indexed_db_backing_store.cc?l=4242 https://codereview.chromium.org/2904603003/diff/1/content/browser/indexed_db/... content/browser/indexed_db/leveldb/leveldb_transaction.cc:381: transaction_->size_ -= data_iterator_->Value().size(); IIUC, this removes the record from the map, so I think you need to subtract sizeof(Record) + key.size() * 2 + value->size(). https://codereview.chromium.org/2904603003/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp (right): https://codereview.chromium.org/2904603003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp:516: Could you please also UMA the number of Blobs attached to a value?
The CQ bit was checked by dmurph@chromium.org to run a CQ dry run
Thanks! Can you PTAL again? https://codereview.chromium.org/2904603003/diff/1/content/browser/indexed_db/... File content/browser/indexed_db/indexed_db_transaction.cc (right): https://codereview.chromium.org/2904603003/diff/1/content/browser/indexed_db/... content/browser/indexed_db/indexed_db_transaction.cc:357: UMA_HISTOGRAM_MEMORY_KB("WebCore.IndexedDB.TransactionSize", On 2017/05/23 23:01:09, pwnall wrote: > Is this number different because of the blob journal changes in CommitPhaseTwo? > Can you please add a comment explaining? Had this wrong - put in right spot now, with comment. https://codereview.chromium.org/2904603003/diff/1/content/browser/indexed_db/... File content/browser/indexed_db/leveldb/leveldb_transaction.cc (right): https://codereview.chromium.org/2904603003/diff/1/content/browser/indexed_db/... content/browser/indexed_db/leveldb/leveldb_transaction.cc:57: size_ += sizeof(Record) + key.size() * 2 + value->size(); On 2017/05/23 23:01:09, pwnall wrote: > This book-keeping looks subtle enough to require tests. Can you add some quick > unit tests today? If not, it seems fine to land this now and potentially fix > later. > > FWIW, I was about to suggest the following plan, and my idea fell flat on its > face because LevelDB doesn't report WriteBatch size. > > 1) Forward the WriteBatch size getter in LevelDBWriteBatch -- > https://cs.chromium.org/chromium/src/content/browser/indexed_db/leveldb/level... > > 2) Change LevelDBTransaction::Commit to return the WriteBatch size in an > out-parameter -- > https://cs.chromium.org/chromium/src/content/browser/indexed_db/leveldb/level... > > 3) Log the value in IndexedDBBackingStore::Transaction::CommitPhaseTwo > --https://cs.chromium.org/chromium/src/content/browser/indexed_db/indexed_db_backing_store.cc?l=4242 BAsed on our convo, recording right before the journal commit, and I'll follow up with blob metrics in a new CL. https://codereview.chromium.org/2904603003/diff/1/content/browser/indexed_db/... content/browser/indexed_db/leveldb/leveldb_transaction.cc:381: transaction_->size_ -= data_iterator_->Value().size(); On 2017/05/23 23:01:10, pwnall wrote: > IIUC, this removes the record from the map, so I think you need to subtract > sizeof(Record) + key.size() * 2 + value->size(). Added tests. Offline convo - record isn't removed from map, just the value. https://codereview.chromium.org/2904603003/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp (right): https://codereview.chromium.org/2904603003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp:516: On 2017/05/23 23:01:10, pwnall wrote: > Could you please also UMA the number of Blobs attached to a value? Done.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM. Thank you for adding these metrics!
The CQ bit was checked by dmurph@chromium.org to run a CQ dry run
dmurph@chromium.org changed reviewers: + mpearson@google.com
+ Mark for histograms review. I'm trying to get this in before Thursday evening (branch point). If you're busy let me know, I can add someone else :)
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 commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
mpearson@chromium.org changed reviewers: + mpearson@chromium.org
https://codereview.chromium.org/2904603003/diff/40001/content/browser/indexed... File content/browser/indexed_db/indexed_db_transaction.cc (right): https://codereview.chromium.org/2904603003/diff/40001/content/browser/indexed... content/browser/indexed_db/indexed_db_transaction.cc:365: UMA_HISTOGRAM_MEDIUM_TIMES(time_uma_name, I don't think these macros will work. Have you tested this? I need think you need to use FactoryGet directly. (time_uma_name can have different values at different times. I think the caching done in the macro will cause an emit to go to the wrong histogram.) https://codereview.chromium.org/2904603003/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2904603003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:80752: +<histogram name="WebCore.IndexedDB.Transaction.VersionChange.SizeOnCommit" These two description below seem not applicable to the VersionChange histograms they're attached to.
Thanks Mark! Can you ptal again? https://codereview.chromium.org/2904603003/diff/40001/content/browser/indexed... File content/browser/indexed_db/indexed_db_transaction.cc (right): https://codereview.chromium.org/2904603003/diff/40001/content/browser/indexed... content/browser/indexed_db/indexed_db_transaction.cc:365: UMA_HISTOGRAM_MEDIUM_TIMES(time_uma_name, On 2017/05/24 18:57:38, Mark P wrote: > I don't think these macros will work. Have you tested this? I need think you > need to use FactoryGet directly. > (time_uma_name can have different values at different times. I think the > caching done in the macro will cause an emit to go to the wrong histogram.) Yep, fixed. https://codereview.chromium.org/2904603003/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2904603003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:80752: +<histogram name="WebCore.IndexedDB.Transaction.VersionChange.SizeOnCommit" On 2017/05/24 18:57:38, Mark P wrote: > These two description below seem not applicable to the VersionChange histograms > they're attached to. Ah! thanks. Added short description about when versionchange txns happen as well (it's part of spec, but doesn't hurt).
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...
histograms.xml lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dmurph@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pwnall@chromium.org, mpearson@chromium.org Link to the patchset: https://codereview.chromium.org/2904603003/#ps80001 (title: "updated fakes and size interface to let tests pass")
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 commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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...
The CQ bit was unchecked by dmurph@chromium.org
The CQ bit was checked by dmurph@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mpearson@chromium.org, pwnall@chromium.org Link to the patchset: https://codereview.chromium.org/2904603003/#ps100001 (title: "fixed tests, ran tests")
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 commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Turns out I'm using sizeof(Record), which depends on the system, so my tests can't be hardcoded numbers. Whoops! I guess I'll have to do the math in the test :P
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...
The CQ bit was unchecked by dmurph@chromium.org
The CQ bit was checked by dmurph@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mpearson@chromium.org, pwnall@chromium.org Link to the patchset: https://codereview.chromium.org/2904603003/#ps120001 (title: "doing math in test")
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 jsbell@chromium.org
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_headless_rel on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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...)
The CQ bit was checked by dmurph@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mpearson@chromium.org, pwnall@chromium.org Link to the patchset: https://codereview.chromium.org/2904603003/#ps180001 (title: "rebase again")
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 commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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...
The CQ bit was unchecked by dmurph@chromium.org
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...
The CQ bit was unchecked by dmurph@chromium.org
The CQ bit was checked by dmurph@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mpearson@chromium.org, pwnall@chromium.org Link to the patchset: https://codereview.chromium.org/2904603003/#ps200001 (title: "fix for error cases")
The CQ bit was unchecked by dmurph@chromium.org
The CQ bit was checked by dmurph@chromium.org to run a CQ dry run
The CQ bit was unchecked by dmurph@chromium.org
The CQ bit was checked by dmurph@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mpearson@chromium.org, pwnall@chromium.org Link to the patchset: https://codereview.chromium.org/2904603003/#ps220001 (title: "made sure size is 0'd out")
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 commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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...
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1496264461347190,
"parent_rev": "92555328de0f8ad2579d0daab74c2045497de3c3", "commit_rev":
"4dc3da85cabab5ebae5807dbb4ecb3d270e9072b"}
Message was sent while issue was closed.
Description was changed from ========== [IndexedDB] Adding txn, value, and key size metrics, and update txn cost. Metrics for transaction, value, and key sizes. Transaction size cost for quota updated to include key size estimation. BUG=723129 ========== to ========== [IndexedDB] Adding txn, value, and key size metrics, and update txn cost. Metrics for transaction, value, and key sizes. Transaction size cost for quota updated to include key size estimation. BUG=723129 Review-Url: https://codereview.chromium.org/2904603003 Cr-Commit-Position: refs/heads/master@{#476089} Committed: https://chromium.googlesource.com/chromium/src/+/4dc3da85cabab5ebae5807dbb4ec... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as https://chromium.googlesource.com/chromium/src/+/4dc3da85cabab5ebae5807dbb4ec... |
