Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(5)

Issue 2760163002: [IndexedDB] Pool and evict leveldb iterators, to save memory (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 months, 1 week ago by dmurph
Modified:
3 months ago
Reviewers:
pwnall, boliu, cmumford, jsbell
CC:
chromium-reviews, jam, jsbell+idb_chromium.org, darin-cc_chromium.org, cmumford
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

[IndexedDB] Pool and evict leveldb iterators, to save memory Caps the total number of open cursors per database. Ideally the cap would be on the # of bytes held by the cursors, but this data is deeply hidden, and this is good enough for an initial mitigation. Note: This could potentially cause worse memory churn if the number of cursors allowed is too low. R=jsbell,pwnall,cmumford BUG=696055, 702787 Review-Url: https://codereview.chromium.org/2760163002 Cr-Commit-Position: refs/heads/master@{#459927} Committed: https://chromium.googlesource.com/chromium/src/+/ec764054b12a8e21e1fe958c3f1e4d47df671066

Patch Set 1 #

Total comments: 12

Patch Set 2 : Comments #

Patch Set 3 : tests! cleanup! #

Patch Set 4 : whoops, forgot the .cc file #

Patch Set 5 : compile fixed #

Total comments: 20

Patch Set 6 : Moved logic to impl class instead - didn't finish comments #

Patch Set 7 : comments #

Total comments: 13

Patch Set 8 : fixed browsertest compilation #

Patch Set 9 : comments #

Patch Set 10 : fix key access #

Patch Set 11 : comments & lifetime fix #

Total comments: 24

Patch Set 12 : comments & simplifying MRU interaction #

Total comments: 21

Patch Set 13 : comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+304 lines, -47 lines) Patch
M content/browser/BUILD.gn View 1 2 6 7 8 9 10 11 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/indexed_db/indexed_db_backing_store.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +3 lines, -1 line 0 comments Download
M content/browser/indexed_db/indexed_db_backing_store_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +4 lines, -1 line 0 comments Download
M content/browser/indexed_db/indexed_db_class_factory.h View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +4 lines, -1 line 0 comments Download
M content/browser/indexed_db/indexed_db_class_factory.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +5 lines, -2 lines 0 comments Download
M content/browser/indexed_db/indexed_db_cleanup_on_io_error_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +2 lines, -1 line 0 comments Download
M content/browser/indexed_db/leveldb/leveldb_database.h View 1 2 3 4 5 6 7 8 9 10 11 12 6 chunks +42 lines, -1 line 0 comments Download
M content/browser/indexed_db/leveldb/leveldb_database.cc View 1 2 3 4 5 6 7 8 9 10 11 12 6 chunks +46 lines, -5 lines 0 comments Download
M content/browser/indexed_db/leveldb/leveldb_iterator.h View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +7 lines, -0 lines 0 comments Download
A + content/browser/indexed_db/leveldb/leveldb_iterator.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +4 lines, -2 lines 0 comments Download
M content/browser/indexed_db/leveldb/leveldb_iterator_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +23 lines, -2 lines 0 comments Download
M content/browser/indexed_db/leveldb/leveldb_iterator_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +60 lines, -12 lines 0 comments Download
M content/browser/indexed_db/leveldb/leveldb_transaction.h View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +4 lines, -3 lines 0 comments Download
M content/browser/indexed_db/leveldb/leveldb_transaction.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +3 lines, -0 lines 0 comments Download
M content/browser/indexed_db/leveldb/leveldb_transaction_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 5 chunks +70 lines, -3 lines 0 comments Download
M content/browser/indexed_db/leveldb/leveldb_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 5 chunks +9 lines, -6 lines 0 comments Download
M content/browser/indexed_db/mock_browsertest_indexed_db_class_factory.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +3 lines, -1 line 0 comments Download
M content/browser/indexed_db/mock_browsertest_indexed_db_class_factory.cc View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +14 lines, -6 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 81 (58 generated)
dmurph
Chris, Victor, and Josh - can you PTAL? This should limit the # of cursors. ...
3 months, 1 week ago (2017-03-20 23:18:00 UTC) #3
jsbell
Very initial feedback. High-level - this is about what I was expected the fix to ...
3 months, 1 week ago (2017-03-21 00:15:48 UTC) #6
dmurph
Thanks! I'm encouraged by all of the layout tests passing. I'm still trying to figure ...
3 months, 1 week ago (2017-03-21 20:13:26 UTC) #11
jsbell
As discussed offline, tests in content/browser/indexed_db/leveldb/leveldb_transaction_unittest.cc https://codereview.chromium.org/2760163002/diff/1/content/browser/indexed_db/leveldb/leveldb_iterator.h File content/browser/indexed_db/leveldb/leveldb_iterator.h (right): https://codereview.chromium.org/2760163002/diff/1/content/browser/indexed_db/leveldb/leveldb_iterator.h#newcode24 content/browser/indexed_db/leveldb/leveldb_iterator.h:24: virtual void EvictWorkingMemory(){}; ...
3 months, 1 week ago (2017-03-21 20:51:39 UTC) #12
dmurph
Hello all, can you PTAL? I've added tests - the one ugly bit is how ...
3 months ago (2017-03-22 18:35:58 UTC) #18
jsbell
initial feedback - looking good! https://codereview.chromium.org/2760163002/diff/80001/content/browser/indexed_db/leveldb/leveldb_database.cc File content/browser/indexed_db/leveldb/leveldb_database.cc (right): https://codereview.chromium.org/2760163002/diff/80001/content/browser/indexed_db/leveldb/leveldb_database.cc#newcode44 content/browser/indexed_db/leveldb/leveldb_database.cc:44: static const size_t kDefaultMaxOpenIteratorsPerDatabase ...
3 months ago (2017-03-22 23:55:32 UTC) #29
dmurph
Josh & Victor: PTAL. I addressed comments and also re-architecture'd this to just live in ...
3 months ago (2017-03-23 20:56:48 UTC) #32
jsbell
https://codereview.chromium.org/2760163002/diff/120001/content/browser/indexed_db/leveldb/leveldb_database.cc File content/browser/indexed_db/leveldb/leveldb_database.cc (right): https://codereview.chromium.org/2760163002/diff/120001/content/browser/indexed_db/leveldb/leveldb_database.cc#newcode357 content/browser/indexed_db/leveldb/leveldb_database.cc:357: base::WrapUnique(new LevelDBDatabase(NO_CURSOR_EVICTION)); Interesting... I suppose we should check Incognito ...
3 months ago (2017-03-23 21:48:22 UTC) #35
dmurph
Thanks for comments. PTAL. https://codereview.chromium.org/2760163002/diff/120001/content/browser/indexed_db/leveldb/leveldb_database.cc File content/browser/indexed_db/leveldb/leveldb_database.cc (right): https://codereview.chromium.org/2760163002/diff/120001/content/browser/indexed_db/leveldb/leveldb_database.cc#newcode357 content/browser/indexed_db/leveldb/leveldb_database.cc:357: base::WrapUnique(new LevelDBDatabase(NO_CURSOR_EVICTION)); On 2017/03/23 21:48:21, ...
3 months ago (2017-03-23 22:48:47 UTC) #43
jsbell
https://codereview.chromium.org/2760163002/diff/120001/content/browser/indexed_db/leveldb/leveldb_iterator_impl.cc File content/browser/indexed_db/leveldb/leveldb_iterator_impl.cc (right): https://codereview.chromium.org/2760163002/diff/120001/content/browser/indexed_db/leveldb/leveldb_iterator_impl.cc#newcode123 content/browser/indexed_db/leveldb/leveldb_iterator_impl.cc:123: } On 2017/03/23 22:48:47, dmurph wrote: > On 2017/03/23 ...
3 months ago (2017-03-24 00:30:21 UTC) #47
pwnall
The CL is surprisingly un-intrusive for all it does! Nice! https://codereview.chromium.org/2760163002/diff/200001/content/browser/indexed_db/indexed_db_backing_store.cc File content/browser/indexed_db/indexed_db_backing_store.cc (right): https://codereview.chromium.org/2760163002/diff/200001/content/browser/indexed_db/indexed_db_backing_store.cc#newcode61 ...
3 months ago (2017-03-24 09:16:03 UTC) #52
pwnall
I'd have used "IndexedDB: Pool and evict LevelDB iterators, to save memory" as the commit ...
3 months ago (2017-03-24 09:18:19 UTC) #53
dmurph
Let me know what you think! I went the struct route - what do you ...
3 months ago (2017-03-24 23:33:40 UTC) #56
jsbell
lgtm with just nits https://codereview.chromium.org/2760163002/diff/220001/content/browser/indexed_db/leveldb/leveldb_database.cc File content/browser/indexed_db/leveldb/leveldb_database.cc (right): https://codereview.chromium.org/2760163002/diff/220001/content/browser/indexed_db/leveldb/leveldb_database.cc#newcode357 content/browser/indexed_db/leveldb/leveldb_database.cc:357: // Since the database is ...
3 months ago (2017-03-27 15:35:34 UTC) #59
pwnall
https://codereview.chromium.org/2760163002/diff/200001/content/browser/indexed_db/leveldb/leveldb_database.cc File content/browser/indexed_db/leveldb/leveldb_database.cc (right): https://codereview.chromium.org/2760163002/diff/200001/content/browser/indexed_db/leveldb/leveldb_database.cc#newcode447 content/browser/indexed_db/leveldb/leveldb_database.cc:447: read_options.snapshot)); On 2017/03/24 23:33:39, dmurph wrote: > On 2017/03/24 ...
3 months ago (2017-03-27 17:08:04 UTC) #60
dmurph
https://codereview.chromium.org/2760163002/diff/200001/content/browser/indexed_db/leveldb/leveldb_database.cc File content/browser/indexed_db/leveldb/leveldb_database.cc (right): https://codereview.chromium.org/2760163002/diff/200001/content/browser/indexed_db/leveldb/leveldb_database.cc#newcode447 content/browser/indexed_db/leveldb/leveldb_database.cc:447: read_options.snapshot)); On 2017/03/27 17:08:03, pwnall wrote: > On 2017/03/24 ...
3 months ago (2017-03-27 21:32:05 UTC) #65
pwnall
LGTM https://codereview.chromium.org/2760163002/diff/220001/content/browser/indexed_db/leveldb/leveldb_iterator.h File content/browser/indexed_db/leveldb/leveldb_iterator.h (right): https://codereview.chromium.org/2760163002/diff/220001/content/browser/indexed_db/leveldb/leveldb_iterator.h#newcode28 content/browser/indexed_db/leveldb/leveldb_iterator.h:28: virtual void Detach(){}; On 2017/03/27 21:32:05, dmurph wrote: ...
3 months ago (2017-03-27 21:52:18 UTC) #67
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2760163002/240001
3 months ago (2017-03-27 22:28:34 UTC) #71
commit-bot: I haz the power
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_presubmit/builds/395747)
3 months ago (2017-03-27 22:37:32 UTC) #73
dmurph
+boliu: can you PTAL at the content/browser/BUILD.gn change?
3 months ago (2017-03-27 22:38:19 UTC) #75
boliu
On 2017/03/27 22:38:19, dmurph wrote: > +boliu: can you PTAL at the content/browser/BUILD.gn change? rs ...
3 months ago (2017-03-27 22:50:18 UTC) #76
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2760163002/240001
3 months ago (2017-03-27 23:05:53 UTC) #78
commit-bot: I haz the power
3 months ago (2017-03-27 23:14:32 UTC) #81
Message was sent while issue was closed.
Committed patchset #13 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/ec764054b12a8e21e1fe958c3f1e...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cb946e318