Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1803253002:
Improved iterator for persistent memory allocator. (Closed)
|
Created:
4 years, 9 months ago by bcwhite Modified:
4 years, 8 months ago CC:
chromium-reviews, asvitkine+watch_chromium.org, vmpstr+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@refactor-hp Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImproved iterator for persistent memory allocator.
The previous "iterator" was just state that was updated
by the main class which is not generally how iterators
operate. New version creates self-contained objects
that do the "getnext" completely on their own.
Also fixed old bug with incorrect construction parameters
for histograms in test. It never affected operation but
generates VLOG messages and affects timing tests.
BUG=546019
Committed: https://crrev.com/f246202df709348d727c4a47f256a3da1caa0bef
Cr-Commit-Position: refs/heads/master@{#385467}
Patch Set 1 #Patch Set 2 : rebased and fixed up a bit #
Total comments: 18
Patch Set 3 : addressd review comments by Alex #
Total comments: 12
Patch Set 4 : addressd more review comments by Alex #
Total comments: 2
Patch Set 5 : add some changes missed in previous upload (relaxed->release) #Patch Set 6 : rebased #Depends on Patchset: Messages
Total messages: 33 (15 generated)
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
bcwhite@chromium.org changed reviewers: + isherman@chromium.org
There's a fairly long history to the PersistentMemoryAllocator and its lock-free operation. Feel free to chat with me if you have questions -- it may be faster than doing it through the review system. I'm in Montreal, so 3 hours later than you.
Patchset #2 (id:80001) has been deleted
Sorry for the delay -- I didn't quite get to this code review prior to leaving for vacation. Mostly LG, modulo the core code, which I don't feel qualified to review, and some nits: https://codereview.chromium.org/1803253002/diff/100001/base/metrics/persisten... File base/metrics/persistent_histogram_allocator.cc (right): https://codereview.chromium.org/1803253002/diff/100001/base/metrics/persisten... base/metrics/persistent_histogram_allocator.cc:118: PersistentMemoryAllocator::Reference ref; nit: s/PersistentMemoryAllocator::Reference/Reference https://codereview.chromium.org/1803253002/diff/100001/base/metrics/persisten... File base/metrics/persistent_histogram_allocator.h (right): https://codereview.chromium.org/1803253002/diff/100001/base/metrics/persisten... base/metrics/persistent_histogram_allocator.h:32: Iterator(PersistentHistogramAllocator* allocator); nit: explicit https://codereview.chromium.org/1803253002/diff/100001/base/metrics/persisten... base/metrics/persistent_histogram_allocator.h:32: Iterator(PersistentHistogramAllocator* allocator); nit: Please document lifetime expectations for the raw pointer. https://codereview.chromium.org/1803253002/diff/100001/base/metrics/persisten... base/metrics/persistent_histogram_allocator.h:35: // if there are no more histograms to return. This may still be called Optional nit: s/Null will be returned/Returns null https://codereview.chromium.org/1803253002/diff/100001/base/metrics/persisten... File base/metrics/persistent_memory_allocator.cc (right): https://codereview.chromium.org/1803253002/diff/100001/base/metrics/persisten... base/metrics/persistent_memory_allocator.cc:136: last_record_ = kReferenceQueue; FWIW, NOTREACHED() and DCHECK failures should be unhandled, as they document a state that should not be possible. https://codereview.chromium.org/1803253002/diff/100001/base/metrics/persisten... base/metrics/persistent_memory_allocator.cc:219: } Sorry, I don't feel qualified to review this code for correctness. I can try to bring myself up to speed on the intricacies of the C++ memory model next week, but it might be easier to find another reviewer who is already familiar with it. https://codereview.chromium.org/1803253002/diff/100001/base/metrics/persisten... File base/metrics/persistent_memory_allocator.h (right): https://codereview.chromium.org/1803253002/diff/100001/base/metrics/persisten... base/metrics/persistent_memory_allocator.h:64: Iterator(const PersistentMemoryAllocator* allocator); nit: explicit https://codereview.chromium.org/1803253002/diff/100001/base/metrics/persisten... base/metrics/persistent_memory_allocator.h:64: Iterator(const PersistentMemoryAllocator* allocator); nit: Please document lifetime expectations for this raw pointer. https://codereview.chromium.org/1803253002/diff/100001/base/metrics/persisten... File base/metrics/persistent_memory_allocator_unittest.cc (right): https://codereview.chromium.org/1803253002/diff/100001/base/metrics/persisten... base/metrics/persistent_memory_allocator_unittest.cc:584: DCHECK_EQ(filesize != minsize, allocator.IsCorrupt()); Did you mean EXPECT_EQ?
>Sorry for the delay -- I didn't quite get to this code >review prior to leaving for vacation. No problem. I saw that notice from you but there was no rush on it. Thanks for taking care of it... but go back to your vacation. :-) https://codereview.chromium.org/1803253002/diff/100001/base/metrics/persisten... File base/metrics/persistent_histogram_allocator.cc (right): https://codereview.chromium.org/1803253002/diff/100001/base/metrics/persisten... base/metrics/persistent_histogram_allocator.cc:118: PersistentMemoryAllocator::Reference ref; On 2016/03/24 02:03:58, Ilya Sherman (Away Mar 19-27) wrote: > nit: s/PersistentMemoryAllocator::Reference/Reference PHA::Reference is implemented as a PMA::Reference but they're not conceptually the same thing. In this method, I'm iterating over a PMA so want to use PMA::Reference. I'll add a comment about this. https://codereview.chromium.org/1803253002/diff/100001/base/metrics/persisten... File base/metrics/persistent_histogram_allocator.h (right): https://codereview.chromium.org/1803253002/diff/100001/base/metrics/persisten... base/metrics/persistent_histogram_allocator.h:32: Iterator(PersistentHistogramAllocator* allocator); On 2016/03/24 02:03:58, Ilya Sherman (Away Mar 19-27) wrote: > nit: explicit Done. https://codereview.chromium.org/1803253002/diff/100001/base/metrics/persisten... base/metrics/persistent_histogram_allocator.h:32: Iterator(PersistentHistogramAllocator* allocator); On 2016/03/24 02:03:58, Ilya Sherman (Away Mar 19-27) wrote: > nit: Please document lifetime expectations for the raw pointer. Done. https://codereview.chromium.org/1803253002/diff/100001/base/metrics/persisten... base/metrics/persistent_histogram_allocator.h:35: // if there are no more histograms to return. This may still be called On 2016/03/24 02:03:58, Ilya Sherman (Away Mar 19-27) wrote: > Optional nit: s/Null will be returned/Returns null Done. https://codereview.chromium.org/1803253002/diff/100001/base/metrics/persisten... File base/metrics/persistent_memory_allocator.cc (right): https://codereview.chromium.org/1803253002/diff/100001/base/metrics/persisten... base/metrics/persistent_memory_allocator.cc:136: last_record_ = kReferenceQueue; On 2016/03/24 02:03:58, Ilya Sherman (Away Mar 19-27) wrote: > FWIW, NOTREACHED() and DCHECK failures should be unhandled, as they document a > state that should not be possible. In a properly running instance, this should never happen. However, the allocator deals with communication between the (untrusted) Renderer and the Browser, and so has to be hardened against anything a compromised renderer could do out in the field. https://codereview.chromium.org/1803253002/diff/100001/base/metrics/persisten... base/metrics/persistent_memory_allocator.cc:219: } On 2016/03/24 02:03:58, Ilya Sherman (Away Mar 19-27) wrote: > Sorry, I don't feel qualified to review this code for correctness. I can try to > bring myself up to speed on the intricacies of the C++ memory model next week, > but it might be easier to find another reviewer who is already familiar with it. Will do. https://codereview.chromium.org/1803253002/diff/100001/base/metrics/persisten... File base/metrics/persistent_memory_allocator.h (right): https://codereview.chromium.org/1803253002/diff/100001/base/metrics/persisten... base/metrics/persistent_memory_allocator.h:64: Iterator(const PersistentMemoryAllocator* allocator); On 2016/03/24 02:03:58, Ilya Sherman (Away Mar 19-27) wrote: > nit: Please document lifetime expectations for this raw pointer. Done. https://codereview.chromium.org/1803253002/diff/100001/base/metrics/persisten... base/metrics/persistent_memory_allocator.h:64: Iterator(const PersistentMemoryAllocator* allocator); On 2016/03/24 02:03:58, Ilya Sherman (Away Mar 19-27) wrote: > nit: explicit Done. https://codereview.chromium.org/1803253002/diff/100001/base/metrics/persisten... File base/metrics/persistent_memory_allocator_unittest.cc (right): https://codereview.chromium.org/1803253002/diff/100001/base/metrics/persisten... base/metrics/persistent_memory_allocator_unittest.cc:584: DCHECK_EQ(filesize != minsize, allocator.IsCorrupt()); On 2016/03/24 02:03:58, Ilya Sherman (Away Mar 19-27) wrote: > Did you mean EXPECT_EQ? Done. (Was changed to make it easier to find a problem during development.)
bcwhite@chromium.org changed reviewers: + glider@chromium.org
Alex, would you mind taking a look at a small addition to the PersistentMemoryAllocator? I refactored the iterator, making it also thread-safe (and lock-free). Thanks!
Looks mostly good. Sorry about the delay. https://codereview.chromium.org/1803253002/diff/120001/base/metrics/histogram... File base/metrics/histogram_unittest.cc (right): https://codereview.chromium.org/1803253002/diff/120001/base/metrics/histogram... base/metrics/histogram_unittest.cc:618: Histogram::FactoryGet(histogram_names[i], 1, 100, 10, OOC, what is this change about? Is it related to the refactoring? https://codereview.chromium.org/1803253002/diff/120001/base/metrics/persisten... File base/metrics/persistent_histogram_allocator.h (right): https://codereview.chromium.org/1803253002/diff/120001/base/metrics/persisten... base/metrics/persistent_histogram_allocator.h:55: // It is lock-free and thread-safe which is why this class is also so. s/so/such ? https://codereview.chromium.org/1803253002/diff/120001/base/metrics/persisten... File base/metrics/persistent_histogram_allocator_unittest.cc (right): https://codereview.chromium.org/1803253002/diff/120001/base/metrics/persisten... base/metrics/persistent_histogram_allocator_unittest.cc:91: EXPECT_NE(0U, iter.GetNext(&type)); // Histogram How about adding a multithreaded test for the iterator? https://codereview.chromium.org/1803253002/diff/120001/base/metrics/persisten... File base/metrics/persistent_memory_allocator.cc (right): https://codereview.chromium.org/1803253002/diff/120001/base/metrics/persisten... base/metrics/persistent_memory_allocator.cc:134: if (!block || block->next.load() == 0) { next.load() stands for next.load(std::memory_order_seq_cst). I don't think you really need sequential consistency here (or anywhere in the file). Should be ok to use a relaxed load here. https://codereview.chromium.org/1803253002/diff/120001/base/metrics/persisten... base/metrics/persistent_memory_allocator.cc:136: last_record_ = kReferenceQueue; last_record_ is an atomic, so you need an atomic store here. Also, is it possible to write a test that executes this path? https://codereview.chromium.org/1803253002/diff/120001/base/metrics/persisten... base/metrics/persistent_memory_allocator.cc:153: // mistakenly determining that a loop exists. Isn't this stuff fun? Typo: two spaces. https://codereview.chromium.org/1803253002/diff/120001/base/metrics/persisten... base/metrics/persistent_memory_allocator.cc:156: Reference last = last_record_.load(); An acquire load should be enough here. https://codereview.chromium.org/1803253002/diff/120001/base/metrics/persisten... base/metrics/persistent_memory_allocator.cc:202: allocator_->shared_meta()->freeptr.load(std::memory_order_acquire), Ok to make this std::memory_order_relaxed. https://codereview.chromium.org/1803253002/diff/120001/base/metrics/persisten... base/metrics/persistent_memory_allocator.cc:660: uint32_t freeptr = std::min(shared_meta()->freeptr.load(), mem_size_); In this case a relaxed load is fine.
Description was changed from ========== Improved iterator for persistent memory allocator. The previous "iterator" was just state that was updated by the main class which is not generally how iterators operate. New version creates self-contained objects that do the "getnext" completely on their own. BUG=546019 ========== to ========== Improved iterator for persistent memory allocator. The previous "iterator" was just state that was updated by the main class which is not generally how iterators operate. New version creates self-contained objects that do the "getnext" completely on their own. Also fixed old bug with incorrect construction parameters for histograms in test. It never affected operation but generates VLOG messages and affects timing tests. BUG=546019 ==========
Patchset #3 (id:120001) has been deleted
Patchset #3 (id:140001) has been deleted
Description was changed from ========== Improved iterator for persistent memory allocator. The previous "iterator" was just state that was updated by the main class which is not generally how iterators operate. New version creates self-contained objects that do the "getnext" completely on their own. Also fixed old bug with incorrect construction parameters for histograms in test. It never affected operation but generates VLOG messages and affects timing tests. BUG=546019 ========== to ========== Improved iterator for persistent memory allocator. The previous "iterator" was just state that was updated by the main class which is not generally how iterators operate. New version creates self-contained objects that do the "getnext" completely on their own. Also fixed old bug with incorrect construction parameters for histograms in test. It never affected operation but generates VLOG messages and affects timing tests. BUG=546019 ==========
I mistakenly deleted the patchset on which your comments were based but they've all been addressed. I know that makes it more difficult to verify the recent changes. Sorry.
https://codereview.chromium.org/1803253002/diff/160001/base/metrics/persisten... File base/metrics/persistent_memory_allocator.cc (right): https://codereview.chromium.org/1803253002/diff/160001/base/metrics/persisten... base/metrics/persistent_memory_allocator.cc:136: last_record_.store(kReferenceQueue, std::memory_order_relaxed); std::memory_order_release https://codereview.chromium.org/1803253002/diff/160001/base/metrics/persisten... base/metrics/persistent_memory_allocator.cc:311: shared_meta()->freeptr.store(sizeof(SharedMetadata), For atomic stores, you'll need std::memory_order_release in the cases the data you're writing is a pointer (or is somewhat equal to a pointer, i.e. used to calculate pointers). I guess you need std::memory_order_release here. https://codereview.chromium.org/1803253002/diff/160001/base/metrics/persisten... base/metrics/persistent_memory_allocator.cc:317: shared_meta()->queue.next.store(kReferenceQueue, std::memory_order_relaxed); Should be std::memory_order_release. https://codereview.chromium.org/1803253002/diff/160001/base/metrics/persisten... base/metrics/persistent_memory_allocator.cc:318: shared_meta()->tailptr.store(kReferenceQueue, std::memory_order_relaxed); Ditto https://codereview.chromium.org/1803253002/diff/160001/base/metrics/persisten... base/metrics/persistent_memory_allocator.cc:469: shared_meta()->freeptr.load(std::memory_order_relaxed); Sorry, looks like my previous comment was wrong. |freeptr| is a pointer, and its value is used later, so you need to acquire it.
https://codereview.chromium.org/1803253002/diff/160001/base/metrics/persisten... File base/metrics/persistent_memory_allocator.cc (right): https://codereview.chromium.org/1803253002/diff/160001/base/metrics/persisten... base/metrics/persistent_memory_allocator.cc:136: last_record_.store(kReferenceQueue, std::memory_order_relaxed); On 2016/03/30 16:34:56, Alexander Potapenko wrote: > std::memory_order_release Done. https://codereview.chromium.org/1803253002/diff/160001/base/metrics/persisten... base/metrics/persistent_memory_allocator.cc:311: shared_meta()->freeptr.store(sizeof(SharedMetadata), On 2016/03/30 16:34:56, Alexander Potapenko wrote: > For atomic stores, you'll need std::memory_order_release in the cases the data > you're writing is a pointer (or is somewhat equal to a pointer, i.e. used to > calculate pointers). I guess you need std::memory_order_release here. Even in the constructor where everything is single-threaded? https://codereview.chromium.org/1803253002/diff/160001/base/metrics/persisten... base/metrics/persistent_memory_allocator.cc:469: shared_meta()->freeptr.load(std::memory_order_relaxed); On 2016/03/30 16:34:56, Alexander Potapenko wrote: > Sorry, looks like my previous comment was wrong. > |freeptr| is a pointer, and its value is used later, so you need to acquire it. Done.
https://codereview.chromium.org/1803253002/diff/160001/base/metrics/persisten... File base/metrics/persistent_memory_allocator.cc (right): https://codereview.chromium.org/1803253002/diff/160001/base/metrics/persisten... base/metrics/persistent_memory_allocator.cc:311: shared_meta()->freeptr.store(sizeof(SharedMetadata), On 2016/03/30 17:46:52, bcwhite wrote: > On 2016/03/30 16:34:56, Alexander Potapenko wrote: > > For atomic stores, you'll need std::memory_order_release in the cases the data > > you're writing is a pointer (or is somewhat equal to a pointer, i.e. used to > > calculate pointers). I guess you need std::memory_order_release here. > > Even in the constructor where everything is single-threaded? Ah, you're right. Iff this code is always called before other threads are created, this isn't necessary.
https://codereview.chromium.org/1803253002/diff/160001/base/metrics/persisten... File base/metrics/persistent_memory_allocator.cc (right): https://codereview.chromium.org/1803253002/diff/160001/base/metrics/persisten... base/metrics/persistent_memory_allocator.cc:311: shared_meta()->freeptr.store(sizeof(SharedMetadata), On 2016/03/30 17:50:56, Alexander Potapenko wrote: > On 2016/03/30 17:46:52, bcwhite wrote: > > On 2016/03/30 16:34:56, Alexander Potapenko wrote: > > > For atomic stores, you'll need std::memory_order_release in the cases the > data > > > you're writing is a pointer (or is somewhat equal to a pointer, i.e. used to > > > calculate pointers). I guess you need std::memory_order_release here. > > > > Even in the constructor where everything is single-threaded? > > Ah, you're right. Iff this code is always called before other threads are > created, this isn't necessary. Probably. Best to be safe, though. https://codereview.chromium.org/1803253002/diff/160001/base/metrics/persisten... base/metrics/persistent_memory_allocator.cc:317: shared_meta()->queue.next.store(kReferenceQueue, std::memory_order_relaxed); On 2016/03/30 16:34:56, Alexander Potapenko wrote: > Should be std::memory_order_release. Done. https://codereview.chromium.org/1803253002/diff/160001/base/metrics/persisten... base/metrics/persistent_memory_allocator.cc:318: shared_meta()->tailptr.store(kReferenceQueue, std::memory_order_relaxed); On 2016/03/30 16:34:56, Alexander Potapenko wrote: > Ditto Done.
https://codereview.chromium.org/1803253002/diff/180001/base/metrics/persisten... File base/metrics/persistent_histogram_allocator_unittest.cc (right): https://codereview.chromium.org/1803253002/diff/180001/base/metrics/persisten... base/metrics/persistent_histogram_allocator_unittest.cc:104: recovered = histogram_iter.GetNext(); So are there any actual multithreaded tests for this functionality?
https://codereview.chromium.org/1803253002/diff/180001/base/metrics/persisten... File base/metrics/persistent_histogram_allocator_unittest.cc (right): https://codereview.chromium.org/1803253002/diff/180001/base/metrics/persisten... base/metrics/persistent_histogram_allocator_unittest.cc:104: recovered = histogram_iter.GetNext(); On 2016/03/30 18:22:21, Alexander Potapenko wrote: > So are there any actual multithreaded tests for this functionality? Parallelism is handled by the PersistentMemoryAllocator. See IteratorParallelismTest. This class just rides on its coattails.
On 2016/03/30 19:51:12, bcwhite wrote: > https://codereview.chromium.org/1803253002/diff/180001/base/metrics/persisten... > File base/metrics/persistent_histogram_allocator_unittest.cc (right): > > https://codereview.chromium.org/1803253002/diff/180001/base/metrics/persisten... > base/metrics/persistent_histogram_allocator_unittest.cc:104: recovered = > histogram_iter.GetNext(); > On 2016/03/30 18:22:21, Alexander Potapenko wrote: > > So are there any actual multithreaded tests for this functionality? > > Parallelism is handled by the PersistentMemoryAllocator. See > IteratorParallelismTest. This class just rides on its coattails. Ok, lgtm
On 2016/03/30 20:52:35, Alexander Potapenko wrote: > On 2016/03/30 19:51:12, bcwhite wrote: > > > https://codereview.chromium.org/1803253002/diff/180001/base/metrics/persisten... > > File base/metrics/persistent_histogram_allocator_unittest.cc (right): > > > > > https://codereview.chromium.org/1803253002/diff/180001/base/metrics/persisten... > > base/metrics/persistent_histogram_allocator_unittest.cc:104: recovered = > > histogram_iter.GetNext(); > > On 2016/03/30 18:22:21, Alexander Potapenko wrote: > > > So are there any actual multithreaded tests for this functionality? > > > > Parallelism is handled by the PersistentMemoryAllocator. See > > IteratorParallelismTest. This class just rides on its coattails. > > Ok, lgtm Thanks. Ilya, back to you...
LGTM, thanks
Patchset #6 (id:220001) has been deleted
The CQ bit was checked by bcwhite@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from glider@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/1803253002/#ps240001 (title: "rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1803253002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1803253002/240001
Message was sent while issue was closed.
Description was changed from ========== Improved iterator for persistent memory allocator. The previous "iterator" was just state that was updated by the main class which is not generally how iterators operate. New version creates self-contained objects that do the "getnext" completely on their own. Also fixed old bug with incorrect construction parameters for histograms in test. It never affected operation but generates VLOG messages and affects timing tests. BUG=546019 ========== to ========== Improved iterator for persistent memory allocator. The previous "iterator" was just state that was updated by the main class which is not generally how iterators operate. New version creates self-contained objects that do the "getnext" completely on their own. Also fixed old bug with incorrect construction parameters for histograms in test. It never affected operation but generates VLOG messages and affects timing tests. BUG=546019 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== Improved iterator for persistent memory allocator. The previous "iterator" was just state that was updated by the main class which is not generally how iterators operate. New version creates self-contained objects that do the "getnext" completely on their own. Also fixed old bug with incorrect construction parameters for histograms in test. It never affected operation but generates VLOG messages and affects timing tests. BUG=546019 ========== to ========== Improved iterator for persistent memory allocator. The previous "iterator" was just state that was updated by the main class which is not generally how iterators operate. New version creates self-contained objects that do the "getnext" completely on their own. Also fixed old bug with incorrect construction parameters for histograms in test. It never affected operation but generates VLOG messages and affects timing tests. BUG=546019 Committed: https://crrev.com/f246202df709348d727c4a47f256a3da1caa0bef Cr-Commit-Position: refs/heads/master@{#385467} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/f246202df709348d727c4a47f256a3da1caa0bef Cr-Commit-Position: refs/heads/master@{#385467} |
