|
|
Description[IndexedDB] Transaction limiting, minimal patch.
Limits the # of active transactions per-database. Prevents
transaction trashing.
R=cmumford,pwnall
BUG=533648
Review-Url: https://codereview.chromium.org/2691423005
Cr-Commit-Position: refs/heads/master@{#451174}
Committed: https://chromium.googlesource.com/chromium/src/+/635bab41ab657fb272b5e1539b194e12e0bffa6f
Patch Set 1 #
Total comments: 5
Patch Set 2 : removed code #Patch Set 3 : more explanation #
Total comments: 5
Patch Set 4 : comment change #Messages
Total messages: 27 (15 generated)
Description was changed from ========== [IndexedDB] Transaction limitting This includes a lot of tracing additions as well for debugging. R=cmumford,pwnall BUG=533648 ========== to ========== [IndexedDB] Transaction limiting, minimal patch. Limits the # of active transactions per-database. Prevents transaction trashing. R=cmumford,pwnall BUG=533648 ==========
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...
PTAL, this is the transaction throttling patch.
Did you leave the extra tracing code in by accident? This doesn't seem minimal to me.
https://codereview.chromium.org/2691423005/diff/1/content/browser/indexed_db/... File content/browser/indexed_db/indexed_db_transaction_coordinator.cc (right): https://codereview.chromium.org/2691423005/diff/1/content/browser/indexed_db/... content/browser/indexed_db/indexed_db_transaction_coordinator.cc:14: static const size_t kMaxStartedTransactions = 10; Can you add a comment explaining why this has a value of 10? https://codereview.chromium.org/2691423005/diff/1/content/browser/indexed_db/... content/browser/indexed_db/indexed_db_transaction_coordinator.cc:162: size_t num_started_txns) const { This is a non-static member function. Why not just access started_transactions_ directly? Seems unnecessarily complex.
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...
https://codereview.chromium.org/2691423005/diff/1/content/browser/indexed_db/... File content/browser/indexed_db/indexed_db_transaction_coordinator.cc (right): https://codereview.chromium.org/2691423005/diff/1/content/browser/indexed_db/... content/browser/indexed_db/indexed_db_transaction_coordinator.cc:14: static const size_t kMaxStartedTransactions = 10; Can you explain that this limits the number of transactions per database? It'd also help if you mention what happens when the limit is reached -- seems like new transactions will remain queued?
The CQ bit was checked by dmurph@chromium.org to run a CQ dry run
https://codereview.chromium.org/2691423005/diff/1/content/browser/indexed_db/... File content/browser/indexed_db/indexed_db_transaction_coordinator.cc (right): https://codereview.chromium.org/2691423005/diff/1/content/browser/indexed_db/... content/browser/indexed_db/indexed_db_transaction_coordinator.cc:14: static const size_t kMaxStartedTransactions = 10; On 2017/02/16 23:19:40, pwnall wrote: > Can you explain that this limits the number of transactions per database? It'd > also help if you mention what happens when the limit is reached -- seems like > new transactions will remain queued? Done. https://codereview.chromium.org/2691423005/diff/1/content/browser/indexed_db/... content/browser/indexed_db/indexed_db_transaction_coordinator.cc:162: size_t num_started_txns) const { On 2017/02/16 23:16:05, cmumford wrote: > This is a non-static member function. Why not just access started_transactions_ > directly? Seems unnecessarily complex. Whoops, yes much better.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm % comment nit. https://codereview.chromium.org/2691423005/diff/40001/content/browser/indexed... File content/browser/indexed_db/indexed_db_transaction_coordinator.cc (right): https://codereview.chromium.org/2691423005/diff/40001/content/browser/indexed... content/browser/indexed_db/indexed_db_transaction_coordinator.cc:15: // Limited prevent transaction trashing. Ten is chosen to reduce performance "Limited to prevent"
https://codereview.chromium.org/2691423005/diff/40001/content/browser/indexed... File content/browser/indexed_db/indexed_db_transaction_coordinator.cc (right): https://codereview.chromium.org/2691423005/diff/40001/content/browser/indexed... content/browser/indexed_db/indexed_db_transaction_coordinator.cc:15: // Limited prevent transaction trashing. Ten is chosen to reduce performance Is this accurate? I thought we're limiting txns to prevent the browser process from consuming unbounded amounts of RAM. https://codereview.chromium.org/2691423005/diff/40001/content/browser/indexed... content/browser/indexed_db/indexed_db_transaction_coordinator.cc:17: // TODO(dmurph): Choose this number better, or implement a scheduler. crbug in the todo?
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...
https://codereview.chromium.org/2691423005/diff/40001/content/browser/indexed... File content/browser/indexed_db/indexed_db_transaction_coordinator.cc (right): https://codereview.chromium.org/2691423005/diff/40001/content/browser/indexed... content/browser/indexed_db/indexed_db_transaction_coordinator.cc:15: // Limited prevent transaction trashing. Ten is chosen to reduce performance On 2017/02/16 23:38:28, pwnall wrote: > Is this accurate? I thought we're limiting txns to prevent the browser process > from consuming unbounded amounts of RAM. That's a side effect of the transaction trashing. https://codereview.chromium.org/2691423005/diff/40001/content/browser/indexed... content/browser/indexed_db/indexed_db_transaction_coordinator.cc:17: // TODO(dmurph): Choose this number better, or implement a scheduler. On 2017/02/16 23:38:28, pwnall wrote: > crbug in the todo? Done.
LGTM, but I think you should create a dependent CL that tests this rate-limiting. The dependent CL doesn't need to be merged to M57/M58, but it can keep the master branch sane.
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 cmumford@chromium.org Link to the patchset: https://codereview.chromium.org/2691423005/#ps60001 (title: "comment change")
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": 60001, "attempt_start_ts": 1487290538398470, "parent_rev": "0d0d5e9eec2e42fddd20050246327e6159702622", "commit_rev": "635bab41ab657fb272b5e1539b194e12e0bffa6f"}
Message was sent while issue was closed.
Description was changed from ========== [IndexedDB] Transaction limiting, minimal patch. Limits the # of active transactions per-database. Prevents transaction trashing. R=cmumford,pwnall BUG=533648 ========== to ========== [IndexedDB] Transaction limiting, minimal patch. Limits the # of active transactions per-database. Prevents transaction trashing. R=cmumford,pwnall BUG=533648 Review-Url: https://codereview.chromium.org/2691423005 Cr-Commit-Position: refs/heads/master@{#451174} Committed: https://chromium.googlesource.com/chromium/src/+/635bab41ab657fb272b5e1539b19... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/635bab41ab657fb272b5e1539b19... |