|
|
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 #Messages
Total messages: 81 (58 generated)
Description was changed from ========== [LevelDB] Eviction for cursor data (w/ recovery) R=jsbell,pwnall,cmumford BUG=696055 ========== to ========== [LevelDB] Eviction for cursor data (w/ recovery) 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 ==========
Description was changed from ========== [LevelDB] Eviction for cursor data (w/ recovery) 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 ========== to ========== [LevelDB] Eviction for cursor data (w/ recovery) 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. TODO: tests - looking for sanity comments right now, working on tests. R=jsbell,pwnall,cmumford BUG=696055 ==========
Chris, Victor, and Josh - can you PTAL? This should limit the # of cursors. I'm currently looking for sanity comments - testing will be in the next patch.
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...
Very initial feedback. High-level - this is about what I was expected the fix to look like. So yay! I'll have to go through it in more detail tomorrow; more documentation and assertions about the internal consistency of the iterator classes is desperately needed. (I ran into this with my attempts last month.) https://codereview.chromium.org/2760163002/diff/1/content/browser/indexed_db/... File content/browser/indexed_db/leveldb/leveldb_database.cc (right): https://codereview.chromium.org/2760163002/diff/1/content/browser/indexed_db/... content/browser/indexed_db/leveldb/leveldb_database.cc:44: static const size_t kMaxOpenIteratorsPerDatabase = 50; We should get UMA stats on this. It may be that we can make the number much lower without impacting most sites today, and give ourselves more headroom in the future. https://codereview.chromium.org/2760163002/diff/1/content/browser/indexed_db/... content/browser/indexed_db/leveldb/leveldb_database.cc:496: if (it == iterator_lru_.end()) { nit: no need for {} https://codereview.chromium.org/2760163002/diff/1/content/browser/indexed_db/... content/browser/indexed_db/leveldb/leveldb_database.cc:500: LOG(ERROR) << "iterator erased"; nit: obviously remove before landing. :) https://codereview.chromium.org/2760163002/diff/1/content/browser/indexed_db/... File content/browser/indexed_db/leveldb/leveldb_database.h (right): https://codereview.chromium.org/2760163002/diff/1/content/browser/indexed_db/... content/browser/indexed_db/leveldb/leveldb_database.h:122: base::MRUCache<LevelDBIterator*, LevelDBIterator*> iterator_lru_; MRU vs lru ? https://codereview.chromium.org/2760163002/diff/1/content/browser/indexed_db/... File content/browser/indexed_db/leveldb/leveldb_iterator.h (right): https://codereview.chromium.org/2760163002/diff/1/content/browser/indexed_db/... content/browser/indexed_db/leveldb/leveldb_iterator.h:24: virtual void EvictWorkingMemory(){}; I seem to recall that there's a more common name for this operation that the memory folks have plumbed through in various areas. We should try and conform (unless I'm misremembering)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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...
Thanks! I'm encouraged by all of the layout tests passing. I'm still trying to figure out how to test this without spinning up entire database instances - it looks like that might be impossible though. Grrrr. We need more refactoring here haha. https://codereview.chromium.org/2760163002/diff/1/content/browser/indexed_db/... File content/browser/indexed_db/leveldb/leveldb_database.cc (right): https://codereview.chromium.org/2760163002/diff/1/content/browser/indexed_db/... content/browser/indexed_db/leveldb/leveldb_database.cc:44: static const size_t kMaxOpenIteratorsPerDatabase = 50; On 2017/03/21 00:15:48, jsbell wrote: > We should get UMA stats on this. It may be that we can make the number much > lower without impacting most sites today, and give ourselves more headroom in > the future. Hm... so I'll keep track of the max, and then report it when we're destructing maybe? https://codereview.chromium.org/2760163002/diff/1/content/browser/indexed_db/... content/browser/indexed_db/leveldb/leveldb_database.cc:496: if (it == iterator_lru_.end()) { On 2017/03/21 00:15:48, jsbell wrote: > nit: no need for {} Done. https://codereview.chromium.org/2760163002/diff/1/content/browser/indexed_db/... content/browser/indexed_db/leveldb/leveldb_database.cc:500: LOG(ERROR) << "iterator erased"; On 2017/03/21 00:15:48, jsbell wrote: > nit: obviously remove before landing. :) Done. https://codereview.chromium.org/2760163002/diff/1/content/browser/indexed_db/... File content/browser/indexed_db/leveldb/leveldb_database.h (right): https://codereview.chromium.org/2760163002/diff/1/content/browser/indexed_db/... content/browser/indexed_db/leveldb/leveldb_database.h:122: base::MRUCache<LevelDBIterator*, LevelDBIterator*> iterator_lru_; On 2017/03/21 00:15:48, jsbell wrote: > MRU vs lru ? Incorrect type name. Very annoying for me haha. https://codereview.chromium.org/2760163002/diff/1/content/browser/indexed_db/... File content/browser/indexed_db/leveldb/leveldb_iterator.h (right): https://codereview.chromium.org/2760163002/diff/1/content/browser/indexed_db/... content/browser/indexed_db/leveldb/leveldb_iterator.h:24: virtual void EvictWorkingMemory(){}; On 2017/03/21 00:15:48, jsbell wrote: > I seem to recall that there's a more common name for this operation that the > memory folks have plumbed through in various areas. We should try and conform > (unless I'm misremembering) I found memory pressure? https://cs.chromium.org/chromium/src/base/memory/memory_pressure_listener.h?d... So to avoid importing that enum - OnMemoryPressure()?
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/... File content/browser/indexed_db/leveldb/leveldb_iterator.h (right): https://codereview.chromium.org/2760163002/diff/1/content/browser/indexed_db/... content/browser/indexed_db/leveldb/leveldb_iterator.h:24: virtual void EvictWorkingMemory(){}; On 2017/03/21 20:13:26, dmurph wrote: > On 2017/03/21 00:15:48, jsbell wrote: > > I seem to recall that there's a more common name for this operation that the > > memory folks have plumbed through in various areas. We should try and conform > > (unless I'm misremembering) > > I found memory pressure? > https://cs.chromium.org/chromium/src/base/memory/memory_pressure_listener.h?d... > > So to avoid importing that enum - OnMemoryPressure()? I think PurgeMemory() is what I was looking for - that term seems to be used in a bunch of places.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by dmurph@chromium.org to run a CQ dry run
Description was changed from ========== [LevelDB] Eviction for cursor data (w/ recovery) 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. TODO: tests - looking for sanity comments right now, working on tests. R=jsbell,pwnall,cmumford BUG=696055 ========== to ========== [LevelDB] Eviction for cursor data (w/ recovery) 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 ==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hello all, can you PTAL? I've added tests - the one ugly bit is how we create the database. I'd like to change this to the "ideal" testing practice. There are a couple choices: 1. Maybe most ideal - separate the construction of the object from the logic in the Open method, so a test can just construct the object. 2. Just add an argument to the original method and change callsites 3. Add a struct options argument to that constructor - I think there's only one option now, but given a lot of our ideas we might be trying to customize more 4. Add a setter for setting testing constraints. I'm not sure if this is a 'dependency' (thinking from a dependency injection stance) - so maybe just a setter for tests is fine. https://codereview.chromium.org/2760163002/diff/1/content/browser/indexed_db/... File content/browser/indexed_db/leveldb/leveldb_iterator.h (right): https://codereview.chromium.org/2760163002/diff/1/content/browser/indexed_db/... content/browser/indexed_db/leveldb/leveldb_iterator.h:24: virtual void EvictWorkingMemory(){}; On 2017/03/21 20:51:39, jsbell wrote: > On 2017/03/21 20:13:26, dmurph wrote: > > On 2017/03/21 00:15:48, jsbell wrote: > > > I seem to recall that there's a more common name for this operation that the > > > memory folks have plumbed through in various areas. We should try and > conform > > > (unless I'm misremembering) > > > > I found memory pressure? > > > https://cs.chromium.org/chromium/src/base/memory/memory_pressure_listener.h?d... > > > > So to avoid importing that enum - OnMemoryPressure()? > > I think PurgeMemory() is what I was looking for - that term seems to be used in > a bunch of places. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) 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 commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
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: This issue passed the CQ dry run.
initial feedback - looking good! https://codereview.chromium.org/2760163002/diff/80001/content/browser/indexed... File content/browser/indexed_db/leveldb/leveldb_database.cc (right): https://codereview.chromium.org/2760163002/diff/80001/content/browser/indexed... content/browser/indexed_db/leveldb/leveldb_database.cc:44: static const size_t kDefaultMaxOpenIteratorsPerDatabase = 50; We should note that this applies only to iterators managed by LevelDBTransaction. Iterators opened directly against the database are not counted, but there should be a bounded number of those, not under the control of script. (Although enumerating those seems wise - I haven't checked myself.) (That said... maybe that's a todo for the future.) https://codereview.chromium.org/2760163002/diff/80001/content/browser/indexed... content/browser/indexed_db/leveldb/leveldb_database.cc:295: leveldb::Status LevelDBDatabase::OpenForTesting( Too much duplicated code. :( Can we refactor? I don't object to changing the signature of LevelDBDatabase::Open() - we could pass in an options object and/or move the default # of cursors into IndexedDBBackingStore. Or just pull out the guts of Open() into OpenInternal() https://codereview.chromium.org/2760163002/diff/80001/content/browser/indexed... content/browser/indexed_db/leveldb/leveldb_database.cc:547: auto it = iterator_lru_.Get(iter); Peek() instead of Get(), since we don't need to mess with the ordering? https://codereview.chromium.org/2760163002/diff/80001/content/browser/indexed... File content/browser/indexed_db/leveldb/leveldb_iterator.h (right): https://codereview.chromium.org/2760163002/diff/80001/content/browser/indexed... content/browser/indexed_db/leveldb/leveldb_iterator.h:24: virtual void PurgeMemory(){}; nit: space between ){ https://codereview.chromium.org/2760163002/diff/80001/content/browser/indexed... File content/browser/indexed_db/leveldb/leveldb_transaction.cc (right): https://codereview.chromium.org/2760163002/diff/80001/content/browser/indexed... content/browser/indexed_db/leveldb/leveldb_transaction.cc:45: for (TransactionIterator* iter : iterators_) { I left a comment before... can this be a DCHECK since iterators should not outlive transactions? Or was I wrong? https://codereview.chromium.org/2760163002/diff/80001/content/browser/indexed... content/browser/indexed_db/leveldb/leveldb_transaction.cc:365: if (current_ == nullptr && db_iterator_state_ != DBIteratorState::ACTIVE) { Nit: no need for {} on single line. or do we want DCHECK_EQ(db_iterator_state_, DBIteratorState::EVICTED_AND_VALID) ? https://codereview.chromium.org/2760163002/diff/80001/content/browser/indexed... content/browser/indexed_db/leveldb/leveldb_transaction.cc:372: if (db_iterator_state_ != DBIteratorState::ACTIVE && current_ == nullptr) { This logic is subtle and probably deserves a comment. https://codereview.chromium.org/2760163002/diff/80001/content/browser/indexed... content/browser/indexed_db/leveldb/leveldb_transaction.cc:373: const_cast<LevelDBTransaction::TransactionIterator*>(this) Comment about the const_cast Explain (for future readers) why making members `mutable` is not enough. https://codereview.chromium.org/2760163002/diff/80001/content/browser/indexed... File content/browser/indexed_db/leveldb/leveldb_transaction.h (right): https://codereview.chromium.org/2760163002/diff/80001/content/browser/indexed... content/browser/indexed_db/leveldb/leveldb_transaction.h:130: bool IsMemoryPurged() const override; I think I'd keep this called 'IsEvicted' to be consistent. (Sorry for waffling. I still like PurgeMemory!) https://codereview.chromium.org/2760163002/diff/80001/content/browser/indexed... content/browser/indexed_db/leveldb/leveldb_transaction.h:156: void LoadDBIteratorIfEvicted(); Maybe 'RestoreDBIteratorIfEvicted' ?
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...
Josh & Victor: PTAL. I addressed comments and also re-architecture'd this to just live in the iterator impl. I had to do some other changes to the layers so I could re-create the leveldb::Iterator. https://codereview.chromium.org/2760163002/diff/80001/content/browser/indexed... File content/browser/indexed_db/leveldb/leveldb_database.cc (right): https://codereview.chromium.org/2760163002/diff/80001/content/browser/indexed... content/browser/indexed_db/leveldb/leveldb_database.cc:44: static const size_t kDefaultMaxOpenIteratorsPerDatabase = 50; On 2017/03/22 23:55:32, jsbell wrote: > We should note that this applies only to iterators managed by > LevelDBTransaction. > > Iterators opened directly against the database are not counted, but there should > be a bounded number of those, not under the control of script. (Although > enumerating those seems wise - I haven't checked myself.) > > (That said... maybe that's a todo for the future.) Done. https://codereview.chromium.org/2760163002/diff/80001/content/browser/indexed... content/browser/indexed_db/leveldb/leveldb_database.cc:295: leveldb::Status LevelDBDatabase::OpenForTesting( On 2017/03/22 23:55:32, jsbell wrote: > Too much duplicated code. :( Can we refactor? > > I don't object to changing the signature of LevelDBDatabase::Open() - we could > pass in an options object and/or move the default # of cursors into > IndexedDBBackingStore. > > Or just pull out the guts of Open() into OpenInternal() Done. https://codereview.chromium.org/2760163002/diff/80001/content/browser/indexed... content/browser/indexed_db/leveldb/leveldb_database.cc:547: auto it = iterator_lru_.Get(iter); On 2017/03/22 23:55:32, jsbell wrote: > Peek() instead of Get(), since we don't need to mess with the ordering? Done. https://codereview.chromium.org/2760163002/diff/80001/content/browser/indexed... File content/browser/indexed_db/leveldb/leveldb_iterator.h (right): https://codereview.chromium.org/2760163002/diff/80001/content/browser/indexed... content/browser/indexed_db/leveldb/leveldb_iterator.h:24: virtual void PurgeMemory(){}; On 2017/03/22 23:55:32, jsbell wrote: > nit: space between ){ Done. https://codereview.chromium.org/2760163002/diff/80001/content/browser/indexed... File content/browser/indexed_db/leveldb/leveldb_transaction.cc (right): https://codereview.chromium.org/2760163002/diff/80001/content/browser/indexed... content/browser/indexed_db/leveldb/leveldb_transaction.cc:45: for (TransactionIterator* iter : iterators_) { On 2017/03/22 23:55:32, jsbell wrote: > I left a comment before... can this be a DCHECK since iterators should not > outlive transactions? Or was I wrong? removed. https://codereview.chromium.org/2760163002/diff/80001/content/browser/indexed... content/browser/indexed_db/leveldb/leveldb_transaction.cc:365: if (current_ == nullptr && db_iterator_state_ != DBIteratorState::ACTIVE) { On 2017/03/22 23:55:32, jsbell wrote: > Nit: no need for {} on single line. > > or do we want DCHECK_EQ(db_iterator_state_, DBIteratorState::EVICTED_AND_VALID) > ? Changed due to moving to impl. https://codereview.chromium.org/2760163002/diff/80001/content/browser/indexed... content/browser/indexed_db/leveldb/leveldb_transaction.cc:372: if (db_iterator_state_ != DBIteratorState::ACTIVE && current_ == nullptr) { On 2017/03/22 23:55:32, jsbell wrote: > This logic is subtle and probably deserves a comment. Made better by moving to impl. https://codereview.chromium.org/2760163002/diff/80001/content/browser/indexed... content/browser/indexed_db/leveldb/leveldb_transaction.cc:373: const_cast<LevelDBTransaction::TransactionIterator*>(this) On 2017/03/22 23:55:32, jsbell wrote: > Comment about the const_cast > > Explain (for future readers) why making members `mutable` is not enough. done. https://codereview.chromium.org/2760163002/diff/80001/content/browser/indexed... File content/browser/indexed_db/leveldb/leveldb_transaction.h (right): https://codereview.chromium.org/2760163002/diff/80001/content/browser/indexed... content/browser/indexed_db/leveldb/leveldb_transaction.h:130: bool IsMemoryPurged() const override; On 2017/03/22 23:55:32, jsbell wrote: > I think I'd keep this called 'IsEvicted' to be consistent. (Sorry for waffling. > I still like PurgeMemory!) Done. https://codereview.chromium.org/2760163002/diff/80001/content/browser/indexed... content/browser/indexed_db/leveldb/leveldb_transaction.h:156: void LoadDBIteratorIfEvicted(); On 2017/03/22 23:55:32, jsbell wrote: > Maybe 'RestoreDBIteratorIfEvicted' ? Done.
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_...)
https://codereview.chromium.org/2760163002/diff/120001/content/browser/indexe... File content/browser/indexed_db/leveldb/leveldb_database.cc (right): https://codereview.chromium.org/2760163002/diff/120001/content/browser/indexe... content/browser/indexed_db/leveldb/leveldb_database.cc:357: base::WrapUnique(new LevelDBDatabase(NO_CURSOR_EVICTION)); Interesting... I suppose we should check Incognito mode and how we behave there. Does leveldb's memenv behave differently enough that we can skip cursor pooling, or should we use a pool there too? (Either way, comment...) https://codereview.chromium.org/2760163002/diff/120001/content/browser/indexe... File content/browser/indexed_db/leveldb/leveldb_iterator.h (right): https://codereview.chromium.org/2760163002/diff/120001/content/browser/indexe... content/browser/indexed_db/leveldb/leveldb_iterator.h:24: virtual void PurgeMemory(){}; nit: space between ){ https://codereview.chromium.org/2760163002/diff/120001/content/browser/indexe... File content/browser/indexed_db/leveldb/leveldb_iterator_impl.cc (right): https://codereview.chromium.org/2760163002/diff/120001/content/browser/indexe... content/browser/indexed_db/leveldb/leveldb_iterator_impl.cc:82: return MakeStringPiece(iterator_->key()); Doesn't this need to use key_before_eviction_ if evicted? https://codereview.chromium.org/2760163002/diff/120001/content/browser/indexe... content/browser/indexed_db/leveldb/leveldb_iterator_impl.cc:88: // as we're dealing with a caching layer. Can you see if making iterator_state_, key_before_eviction_, and iterator_ mutable is enough to avoid the const_cast ? (They achieve the same thing, so whichever ends up being the most readable) https://codereview.chromium.org/2760163002/diff/120001/content/browser/indexe... content/browser/indexed_db/leveldb/leveldb_iterator_impl.cc:123: } else clause that DCHECK(!IsValid()) ? https://codereview.chromium.org/2760163002/diff/120001/content/browser/indexe... File content/browser/indexed_db/leveldb/leveldb_transaction.h (right): https://codereview.chromium.org/2760163002/diff/120001/content/browser/indexe... content/browser/indexed_db/leveldb/leveldb_transaction.h:125: bool IsEvicted() const override; Is this just for tests?
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: 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 checked by dmurph@chromium.org to run a CQ dry run
Thanks for comments. PTAL. https://codereview.chromium.org/2760163002/diff/120001/content/browser/indexe... File content/browser/indexed_db/leveldb/leveldb_database.cc (right): https://codereview.chromium.org/2760163002/diff/120001/content/browser/indexe... content/browser/indexed_db/leveldb/leveldb_database.cc:357: base::WrapUnique(new LevelDBDatabase(NO_CURSOR_EVICTION)); On 2017/03/23 21:48:21, jsbell wrote: > Interesting... I suppose we should check Incognito mode and how we behave there. > Does leveldb's memenv behave differently enough that we can skip cursor pooling, > or should we use a pool there too? > > (Either way, comment...) Well - cursor pooling would do nothing. Everything is in memory. Unless I'm missing something. https://codereview.chromium.org/2760163002/diff/120001/content/browser/indexe... File content/browser/indexed_db/leveldb/leveldb_iterator.h (right): https://codereview.chromium.org/2760163002/diff/120001/content/browser/indexe... content/browser/indexed_db/leveldb/leveldb_iterator.h:24: virtual void PurgeMemory(){}; On 2017/03/23 21:48:21, jsbell wrote: > nit: space between ){ strangely clang format thinks otherwise. https://codereview.chromium.org/2760163002/diff/120001/content/browser/indexe... File content/browser/indexed_db/leveldb/leveldb_iterator_impl.cc (right): https://codereview.chromium.org/2760163002/diff/120001/content/browser/indexe... content/browser/indexed_db/leveldb/leveldb_iterator_impl.cc:82: return MakeStringPiece(iterator_->key()); On 2017/03/23 21:48:21, jsbell wrote: > Doesn't this need to use key_before_eviction_ if evicted? Ah! Thanks for catching this. Didn't transfer in refactor. Test added. https://codereview.chromium.org/2760163002/diff/120001/content/browser/indexe... content/browser/indexed_db/leveldb/leveldb_iterator_impl.cc:88: // as we're dealing with a caching layer. On 2017/03/23 21:48:21, jsbell wrote: > Can you see if making iterator_state_, key_before_eviction_, and iterator_ > mutable is enough to avoid the const_cast ? > > (They achieve the same thing, so whichever ends up being the most readable) Since this would require making all member variables (except snapshot - which is 4/5) mutable, I'm going to stick with const_cast. https://codereview.chromium.org/2760163002/diff/120001/content/browser/indexe... content/browser/indexed_db/leveldb/leveldb_iterator_impl.cc:123: } On 2017/03/23 21:48:21, jsbell wrote: > else clause that DCHECK(!IsValid()) ? > tautology - we have to be in state EVICTED_AND_INVALID in the else clause - so it would always return true. https://codereview.chromium.org/2760163002/diff/120001/content/browser/indexe... File content/browser/indexed_db/leveldb/leveldb_transaction.h (right): https://codereview.chromium.org/2760163002/diff/120001/content/browser/indexe... content/browser/indexed_db/leveldb/leveldb_transaction.h:125: bool IsEvicted() const override; On 2017/03/23 21:48:21, jsbell wrote: > Is this just for tests? Yes.
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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2760163002/diff/120001/content/browser/indexe... File content/browser/indexed_db/leveldb/leveldb_iterator_impl.cc (right): https://codereview.chromium.org/2760163002/diff/120001/content/browser/indexe... content/browser/indexed_db/leveldb/leveldb_iterator_impl.cc:123: } On 2017/03/23 22:48:47, dmurph wrote: > On 2017/03/23 21:48:21, jsbell wrote: > > else clause that DCHECK(!IsValid()) ? > > > > tautology - we have to be in state EVICTED_AND_INVALID in the else clause - so > it would always return true. Oops - I meant DCHECK(!iterator_->Valid())
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: This issue passed the CQ dry run.
The CL is surprisingly un-intrusive for all it does! Nice! https://codereview.chromium.org/2760163002/diff/200001/content/browser/indexe... File content/browser/indexed_db/indexed_db_backing_store.cc (right): https://codereview.chromium.org/2760163002/diff/200001/content/browser/indexe... content/browser/indexed_db/indexed_db_backing_store.cc:61: static const size_t kDefaultMaxOpenIteratorsPerDatabase = 50; How about explaining that we do this because every iterator hangs on to a leveldb block, which can be large, and linking to http://crbug.com/696055? https://codereview.chromium.org/2760163002/diff/200001/content/browser/indexe... File content/browser/indexed_db/leveldb/leveldb_database.cc (right): https://codereview.chromium.org/2760163002/diff/200001/content/browser/indexe... content/browser/indexed_db/leveldb/leveldb_database.cc:97: iterator_lru_(max_open_iterators) {} Shouldn't the MRUCache be initialized with NO_AUTO_EVICT? I think we'd leak if it would do its own eviction. https://codereview.chromium.org/2760163002/diff/200001/content/browser/indexe... content/browser/indexed_db/leveldb/leveldb_database.cc:447: read_options.snapshot)); Why doesn't the newly created iterator get added to the LRU here? https://codereview.chromium.org/2760163002/diff/200001/content/browser/indexe... File content/browser/indexed_db/leveldb/leveldb_database.h (right): https://codereview.chromium.org/2760163002/diff/200001/content/browser/indexe... content/browser/indexed_db/leveldb/leveldb_database.h:62: NON_EXPORTED_BASE(public LevelDBIteratorPoolController) { I'm not a big fan of the interface and extra name here. It seems reasonable to me that the iterator would have a pointer/reference to its database and that it'd call the methods on it directly. https://codereview.chromium.org/2760163002/diff/200001/content/browser/indexe... content/browser/indexed_db/leveldb/leveldb_database.h:81: enum { NO_CURSOR_EVICTION = 0 }; It seems to me that this option is only used for tests (directly in content/browser/indexed_db/indexed_db_cleanup_on_io_error_unittest.cc, indirectly via LevelDBDatabase::OpenInMemory). How about setting a large enough limit in that test, sticking with the default for in-memory tests, and removing all the logic associated with not having eviction? https://codereview.chromium.org/2760163002/diff/200001/content/browser/indexe... content/browser/indexed_db/leveldb/leveldb_database.h:114: std::unique_ptr<leveldb::Iterator> CreateLevelDBIterator( Would it make sense to have these be private and add IteratorImpl as a friend class? https://codereview.chromium.org/2760163002/diff/200001/content/browser/indexe... content/browser/indexed_db/leveldb/leveldb_database.h:135: base::MRUCache<LevelDBIterator*, LevelDBIterator*> iterator_lru_; Would it make sense to use a HashingMRUCache here? https://codereview.chromium.org/2760163002/diff/200001/content/browser/indexe... File content/browser/indexed_db/leveldb/leveldb_iterator_impl.cc (right): https://codereview.chromium.org/2760163002/diff/200001/content/browser/indexe... content/browser/indexed_db/leveldb/leveldb_iterator_impl.cc:49: RestoreDBIteratorIfEvicted(); I see these two methods together everywhere. Would it make sense to group them together in a single method that'd be either EnsureIteratorNotEvicted() or WillUseDBIterator()? It makes sense to me that the two concepts are coupled -- whenever you'd want to use an iterator, you need to page it in if it's evicted, and you need to bump it to the front of the LRU list if it's not evicted. https://codereview.chromium.org/2760163002/diff/200001/content/browser/indexe... content/browser/indexed_db/leveldb/leveldb_iterator_impl.cc:113: bool LevelDBIteratorImpl::IsEvicted() const { It's surprising to me that you sometimes call this and sometimes compare the state directly with IteratorState::ACTIVE. Can you migrate all the code to one or the other? Note: the implicit comparison in DCHECK_EQ makes sense even if you switch to IsEvicted() everywhere, because it results in better diagnostic messages. https://codereview.chromium.org/2760163002/diff/200001/content/browser/indexe... File content/browser/indexed_db/leveldb/leveldb_iterator_impl.h (right): https://codereview.chromium.org/2760163002/diff/200001/content/browser/indexe... content/browser/indexed_db/leveldb/leveldb_iterator_impl.h:33: void PurgeMemory() override; I saw the earlier comments regarding these two names. I think it's confusing to have PurgeMemory / IsEvicted not be consistent with each other. I'd have said that internal iterator is detached, and comment that detaching is done to save memory, which can be orthogonal to the caching built on top. https://codereview.chromium.org/2760163002/diff/200001/content/browser/indexe... File content/browser/indexed_db/leveldb/leveldb_transaction_unittest.cc (right): https://codereview.chromium.org/2760163002/diff/200001/content/browser/indexe... content/browser/indexed_db/leveldb/leveldb_transaction_unittest.cc:224: const std::string key1("key1"); You don't seem to reuse these strings (which is perfectly fine, the literals are expressive enough). How about inlining them in the Put calls below?
I'd have used "IndexedDB: Pool and evict LevelDB iterators, to save memory" as the commit headline and CL title. Starting with LevelDB leads me to believe that you're changing leveldb itself, which wouldn't be possible in Chromium anyway.
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...
Let me know what you think! I went the struct route - what do you think? https://codereview.chromium.org/2760163002/diff/200001/content/browser/indexe... File content/browser/indexed_db/indexed_db_backing_store.cc (right): https://codereview.chromium.org/2760163002/diff/200001/content/browser/indexe... content/browser/indexed_db/indexed_db_backing_store.cc:61: static const size_t kDefaultMaxOpenIteratorsPerDatabase = 50; On 2017/03/24 09:16:02, pwnall wrote: > How about explaining that we do this because every iterator hangs on to a > leveldb block, which can be large, and linking to http://crbug.com/696055 Done. https://codereview.chromium.org/2760163002/diff/200001/content/browser/indexe... File content/browser/indexed_db/leveldb/leveldb_database.cc (right): https://codereview.chromium.org/2760163002/diff/200001/content/browser/indexe... content/browser/indexed_db/leveldb/leveldb_database.cc:97: iterator_lru_(max_open_iterators) {} On 2017/03/24 09:16:02, pwnall wrote: > Shouldn't the MRUCache be initialized with NO_AUTO_EVICT? I think we'd leak if > it would do its own eviction. Creating our own struct so we do correct eviction. let me know what you think. https://codereview.chromium.org/2760163002/diff/200001/content/browser/indexe... content/browser/indexed_db/leveldb/leveldb_database.cc:447: read_options.snapshot)); On 2017/03/24 09:16:02, pwnall wrote: > Why doesn't the newly created iterator get added to the LRU here? We don't actually allocated memory in the iterator until we do a Seek operation. So I don't add it until it's actually doing a costly operation. Do you think I should change that? https://codereview.chromium.org/2760163002/diff/200001/content/browser/indexe... File content/browser/indexed_db/leveldb/leveldb_database.h (right): https://codereview.chromium.org/2760163002/diff/200001/content/browser/indexe... content/browser/indexed_db/leveldb/leveldb_database.h:62: NON_EXPORTED_BASE(public LevelDBIteratorPoolController) { On 2017/03/24 09:16:02, pwnall wrote: > I'm not a big fan of the interface and extra name here. It seems reasonable to > me that the iterator would have a pointer/reference to its database and that > it'd call the methods on it directly. That sounds fine to me - just trying to make the interface 'clean', but I see how having more interfaces can also be more confusing. https://codereview.chromium.org/2760163002/diff/200001/content/browser/indexe... content/browser/indexed_db/leveldb/leveldb_database.h:81: enum { NO_CURSOR_EVICTION = 0 }; On 2017/03/24 09:16:03, pwnall wrote: > It seems to me that this option is only used for tests (directly in > content/browser/indexed_db/indexed_db_cleanup_on_io_error_unittest.cc, > indirectly via LevelDBDatabase::OpenInMemory). > > How about setting a large enough limit in that test, sticking with the default > for in-memory tests, and removing all the logic associated with not having > eviction? Based on offline conversation, doing it so the behavior doesn't whether we're in memory or not. Removed ability to disable cursor eviction. https://codereview.chromium.org/2760163002/diff/200001/content/browser/indexe... content/browser/indexed_db/leveldb/leveldb_database.h:114: std::unique_ptr<leveldb::Iterator> CreateLevelDBIterator( On 2017/03/24 09:16:02, pwnall wrote: > Would it make sense to have these be private and add IteratorImpl as a friend > class? Sure. https://codereview.chromium.org/2760163002/diff/200001/content/browser/indexe... content/browser/indexed_db/leveldb/leveldb_database.h:135: base::MRUCache<LevelDBIterator*, LevelDBIterator*> iterator_lru_; On 2017/03/24 09:16:03, pwnall wrote: > Would it make sense to use a HashingMRUCache here? Sure. https://codereview.chromium.org/2760163002/diff/200001/content/browser/indexe... File content/browser/indexed_db/leveldb/leveldb_iterator_impl.cc (right): https://codereview.chromium.org/2760163002/diff/200001/content/browser/indexe... content/browser/indexed_db/leveldb/leveldb_iterator_impl.cc:49: RestoreDBIteratorIfEvicted(); On 2017/03/24 09:16:03, pwnall wrote: > I see these two methods together everywhere. Would it make sense to group them > together in a single method that'd be either EnsureIteratorNotEvicted() or > WillUseDBIterator()? > > It makes sense to me that the two concepts are coupled -- whenever you'd want to > use an iterator, you need to page it in if it's evicted, and you need to bump it > to the front of the LRU list if it's not evicted. Done. https://codereview.chromium.org/2760163002/diff/200001/content/browser/indexe... content/browser/indexed_db/leveldb/leveldb_iterator_impl.cc:113: bool LevelDBIteratorImpl::IsEvicted() const { On 2017/03/24 09:16:03, pwnall wrote: > It's surprising to me that you sometimes call this and sometimes compare the > state directly with IteratorState::ACTIVE. Can you migrate all the code to one > or the other? > > Note: the implicit comparison in DCHECK_EQ makes sense even if you switch to > IsEvicted() everywhere, because it results in better diagnostic messages. Done. https://codereview.chromium.org/2760163002/diff/200001/content/browser/indexe... File content/browser/indexed_db/leveldb/leveldb_iterator_impl.h (right): https://codereview.chromium.org/2760163002/diff/200001/content/browser/indexe... content/browser/indexed_db/leveldb/leveldb_iterator_impl.h:33: void PurgeMemory() override; On 2017/03/24 09:16:03, pwnall wrote: > I saw the earlier comments regarding these two names. I think it's confusing to > have PurgeMemory / IsEvicted not be consistent with each other. I'd have said > that internal iterator is detached, and comment that detaching is done to save > memory, which can be orthogonal to the caching built on top. Done. https://codereview.chromium.org/2760163002/diff/200001/content/browser/indexe... File content/browser/indexed_db/leveldb/leveldb_transaction_unittest.cc (right): https://codereview.chromium.org/2760163002/diff/200001/content/browser/indexe... content/browser/indexed_db/leveldb/leveldb_transaction_unittest.cc:224: const std::string key1("key1"); On 2017/03/24 09:16:03, pwnall wrote: > You don't seem to reuse these strings (which is perfectly fine, the literals are > expressive enough). How about inlining them in the Put calls below? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with just nits https://codereview.chromium.org/2760163002/diff/220001/content/browser/indexe... File content/browser/indexed_db/leveldb/leveldb_database.cc (right): https://codereview.chromium.org/2760163002/diff/220001/content/browser/indexe... content/browser/indexed_db/leveldb/leveldb_database.cc:357: // Since the database is in-memory, cursor pooling is pointless. Obsolete comment? https://codereview.chromium.org/2760163002/diff/220001/content/browser/indexe... File content/browser/indexed_db/leveldb/leveldb_iterator.h (right): https://codereview.chromium.org/2760163002/diff/220001/content/browser/indexe... content/browser/indexed_db/leveldb/leveldb_iterator.h:28: virtual void Detach(){}; Nit: space between ) and { Also, since we DCHECK if you call Detach() twice in the impl that should be noted as part of the contract. https://codereview.chromium.org/2760163002/diff/220001/content/browser/indexe... File content/browser/indexed_db/leveldb/leveldb_iterator_impl.cc (right): https://codereview.chromium.org/2760163002/diff/220001/content/browser/indexe... content/browser/indexed_db/leveldb/leveldb_iterator_impl.cc:88: // as we're dealing with a caching layer. nit: dealing with -> implementing ?? https://codereview.chromium.org/2760163002/diff/220001/content/browser/indexe... File content/browser/indexed_db/leveldb/leveldb_iterator_impl.h (right): https://codereview.chromium.org/2760163002/diff/220001/content/browser/indexe... content/browser/indexed_db/leveldb/leveldb_iterator_impl.h:46: void WillUseDBIterator(); Add comment.
https://codereview.chromium.org/2760163002/diff/200001/content/browser/indexe... File content/browser/indexed_db/leveldb/leveldb_database.cc (right): https://codereview.chromium.org/2760163002/diff/200001/content/browser/indexe... 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 09:16:02, pwnall wrote: > > Why doesn't the newly created iterator get added to the LRU here? > > We don't actually allocated memory in the iterator until we do a Seek operation. > So I don't add it until it's actually doing a costly operation. Do you think I > should change that? I would at the very least document the assumption. I guess the main downside is that the system is a tad more difficult to reason about ("we have at most 50 seeked iterators", vs "we have at most 50 iterators") but, as long as you're sure about the resource usage, I suppose this is fine. https://codereview.chromium.org/2760163002/diff/220001/content/browser/indexe... File content/browser/indexed_db/leveldb/leveldb_database.cc (right): https://codereview.chromium.org/2760163002/diff/220001/content/browser/indexe... content/browser/indexed_db/leveldb/leveldb_database.cc:296: size_t max_open_cursors, Sorry I caught this so late, but I think this param should come before result, because it's an input, not an output. https://codereview.chromium.org/2760163002/diff/220001/content/browser/indexe... content/browser/indexed_db/leveldb/leveldb_database.cc:517: void LevelDBDatabase::NotifyIteratorUsed(LevelDBIterator* iter) { I forgot to ask last time -- is Notify a Chromium* or google3 convention? I think I've seen On* more often, but I definitely read more Blink code than Chromium code. https://codereview.chromium.org/2760163002/diff/220001/content/browser/indexe... File content/browser/indexed_db/leveldb/leveldb_database.h (right): https://codereview.chromium.org/2760163002/diff/220001/content/browser/indexe... content/browser/indexed_db/leveldb/leveldb_database.h:88: size_t max_open_cursors, Sorry I caught this so late, but I think this param should come before db, because it's an input, not an output. https://codereview.chromium.org/2760163002/diff/220001/content/browser/indexe... content/browser/indexed_db/leveldb/leveldb_database.h:137: struct DetachIteratorOnDestruct { OnDelete / WhenDestroyed / OnDestruction? Also, does Chromium style forbid any sort of implementation in header files? It seems to me that the methods here are so simple that we'd get less code from an inline rather than a call + function. https://codereview.chromium.org/2760163002/diff/220001/content/browser/indexe... File content/browser/indexed_db/leveldb/leveldb_iterator_impl.cc (right): https://codereview.chromium.org/2760163002/diff/220001/content/browser/indexe... content/browser/indexed_db/leveldb/leveldb_iterator_impl.cc:48: CheckStatus(); I don't know how accepted this is in Chromium / google3... but if you're returning iterator_->status() every time after you call CheckStatus(), would it make sense to have CheckStatus() return the status, so you can use "return CheckStatus()"? https://codereview.chromium.org/2760163002/diff/220001/content/browser/indexe... File content/browser/indexed_db/leveldb/leveldb_iterator_impl.h (right): https://codereview.chromium.org/2760163002/diff/220001/content/browser/indexe... content/browser/indexed_db/leveldb/leveldb_iterator_impl.h:53: // Variables to facilitate memory purging. I'd prefer "state used by" -- I think that variables are generally locals, and these are members / fields.
Description was changed from ========== [LevelDB] Eviction for cursor data (w/ recovery) 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 ========== to ========== [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 ==========
Description was changed from ========== [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 ========== to ========== [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 ==========
Description was changed from ========== [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 ========== to ========== [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 ==========
The CQ bit was checked by dmurph@chromium.org to run a CQ dry run
https://codereview.chromium.org/2760163002/diff/200001/content/browser/indexe... File content/browser/indexed_db/leveldb/leveldb_database.cc (right): https://codereview.chromium.org/2760163002/diff/200001/content/browser/indexe... 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 23:33:39, dmurph wrote: > > On 2017/03/24 09:16:02, pwnall wrote: > > > Why doesn't the newly created iterator get added to the LRU here? > > > > We don't actually allocated memory in the iterator until we do a Seek > operation. > > So I don't add it until it's actually doing a costly operation. Do you think I > > should change that? > > I would at the very least document the assumption. I guess the main downside is > that the system is a tad more difficult to reason about ("we have at most 50 > seeked iterators", vs "we have at most 50 iterators") but, as long as you're > sure about the resource usage, I suppose this is fine. Done. https://codereview.chromium.org/2760163002/diff/220001/content/browser/indexe... File content/browser/indexed_db/leveldb/leveldb_database.cc (right): https://codereview.chromium.org/2760163002/diff/220001/content/browser/indexe... content/browser/indexed_db/leveldb/leveldb_database.cc:296: size_t max_open_cursors, On 2017/03/27 17:08:04, pwnall wrote: > Sorry I caught this so late, but I think this param should come before result, > because it's an input, not an output. Done. https://codereview.chromium.org/2760163002/diff/220001/content/browser/indexe... content/browser/indexed_db/leveldb/leveldb_database.cc:357: // Since the database is in-memory, cursor pooling is pointless. On 2017/03/27 15:35:34, jsbell wrote: > Obsolete comment? Done. https://codereview.chromium.org/2760163002/diff/220001/content/browser/indexe... content/browser/indexed_db/leveldb/leveldb_database.cc:517: void LevelDBDatabase::NotifyIteratorUsed(LevelDBIterator* iter) { On 2017/03/27 17:08:04, pwnall wrote: > I forgot to ask last time -- is Notify a Chromium* or google3 convention? I > think I've seen On* more often, but I definitely read more Blink code than > Chromium code. I think I mostly see On* code being used for internal callback bindings (so you bind a callback to that code, and then send that callback somewhere). We have 200 "Notify.*(" lines, and 1945 "On.*(" in content/browser. So I'll switch to On. https://codereview.chromium.org/2760163002/diff/220001/content/browser/indexe... File content/browser/indexed_db/leveldb/leveldb_database.h (right): https://codereview.chromium.org/2760163002/diff/220001/content/browser/indexe... content/browser/indexed_db/leveldb/leveldb_database.h:88: size_t max_open_cursors, On 2017/03/27 17:08:04, pwnall wrote: > Sorry I caught this so late, but I think this param should come before db, > because it's an input, not an output. Good catch, thanks. https://codereview.chromium.org/2760163002/diff/220001/content/browser/indexe... content/browser/indexed_db/leveldb/leveldb_database.h:137: struct DetachIteratorOnDestruct { On 2017/03/27 17:08:04, pwnall wrote: > OnDelete / WhenDestroyed / OnDestruction? > > Also, does Chromium style forbid any sort of implementation in header files? It > seems to me that the methods here are so simple that we'd get less code from an > inline rather than a call + function. Done - we now only have the destructor in the .cc file, as it needs the leveldb_iterator include file. https://codereview.chromium.org/2760163002/diff/220001/content/browser/indexe... File content/browser/indexed_db/leveldb/leveldb_iterator.h (right): https://codereview.chromium.org/2760163002/diff/220001/content/browser/indexe... content/browser/indexed_db/leveldb/leveldb_iterator.h:28: virtual void Detach(){}; On 2017/03/27 15:35:34, jsbell wrote: > Nit: space between ) and { > > Also, since we DCHECK if you call Detach() twice in the impl that should be > noted as part of the contract. ugh. Darn clang format! Actually, should I just keep it this way because it's clang-formatted? It keeps reverting every time I run git cl format. https://codereview.chromium.org/2760163002/diff/220001/content/browser/indexe... File content/browser/indexed_db/leveldb/leveldb_iterator_impl.cc (right): https://codereview.chromium.org/2760163002/diff/220001/content/browser/indexe... content/browser/indexed_db/leveldb/leveldb_iterator_impl.cc:48: CheckStatus(); On 2017/03/27 17:08:04, pwnall wrote: > I don't know how accepted this is in Chromium / google3... but if you're > returning iterator_->status() every time after you call CheckStatus(), would it > make sense to have CheckStatus() return the status, so you can use "return > CheckStatus()"? sure. https://codereview.chromium.org/2760163002/diff/220001/content/browser/indexe... content/browser/indexed_db/leveldb/leveldb_iterator_impl.cc:88: // as we're dealing with a caching layer. On 2017/03/27 15:35:34, jsbell wrote: > nit: dealing with -> implementing ?? Done. https://codereview.chromium.org/2760163002/diff/220001/content/browser/indexe... File content/browser/indexed_db/leveldb/leveldb_iterator_impl.h (right): https://codereview.chromium.org/2760163002/diff/220001/content/browser/indexe... content/browser/indexed_db/leveldb/leveldb_iterator_impl.h:46: void WillUseDBIterator(); On 2017/03/27 15:35:34, jsbell wrote: > Add comment. Done. https://codereview.chromium.org/2760163002/diff/220001/content/browser/indexe... content/browser/indexed_db/leveldb/leveldb_iterator_impl.h:53: // Variables to facilitate memory purging. On 2017/03/27 17:08:04, pwnall wrote: > I'd prefer "state used by" -- I think that variables are generally locals, and > these are members / fields. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM https://codereview.chromium.org/2760163002/diff/220001/content/browser/indexe... File content/browser/indexed_db/leveldb/leveldb_iterator.h (right): https://codereview.chromium.org/2760163002/diff/220001/content/browser/indexe... content/browser/indexed_db/leveldb/leveldb_iterator.h:28: virtual void Detach(){}; On 2017/03/27 21:32:05, dmurph wrote: > On 2017/03/27 15:35:34, jsbell wrote: > > Nit: space between ) and { > > > > Also, since we DCHECK if you call Detach() twice in the impl that should be > > noted as part of the contract. > > ugh. Darn clang format! Actually, should I just keep it this way because it's > clang-formatted? It keeps reverting every time I run git cl format. I would suggest keeping the code as-is and and filing a bug.
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 jsbell@chromium.org Link to the patchset: https://codereview.chromium.org/2760163002/#ps240001 (title: "comments")
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
dmurph@chromium.org changed reviewers: + boliu@chromium.org
+boliu: can you PTAL at the content/browser/BUILD.gn change?
On 2017/03/27 22:38:19, dmurph wrote: > +boliu: can you PTAL at the content/browser/BUILD.gn change? rs lgtm
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": 240001, "attempt_start_ts": 1490655903600960, "parent_rev": "8c74bf60202e99c7f8fcf765db8cd601693317b1", "commit_rev": "ec764054b12a8e21e1fe958c3f1e4d47df671066"}
Message was sent while issue was closed.
Description was changed from ========== [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 ========== to ========== [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/+/ec764054b12a8e21e1fe958c3f1e... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001) as https://chromium.googlesource.com/chromium/src/+/ec764054b12a8e21e1fe958c3f1e... |