Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2387733002:
Move memory management code into separate class for future reuse. (Closed)
DescriptionMove memory management code into separate class for future reuse.
BUG=620813
Committed: https://crrev.com/dadd3155cd102736da470fc28fc5aae89c7f0ac3
Cr-Commit-Position: refs/heads/master@{#425206}
Patch Set 1 #
Total comments: 12
Patch Set 2 : added comment and tests #
Total comments: 6
Patch Set 3 : use method for determining 'free' type #
Total comments: 3
Patch Set 4 : make 'free' type a ctor parameter #
Total comments: 2
Patch Set 5 : object_free -> object_free_type #
Messages
Total messages: 61 (43 generated)
The CQ bit was checked by bcwhite@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.
bcwhite@chromium.org changed reviewers: + manzagop@chromium.org
The CQ bit was checked by bcwhite@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.
Patchset #2 (id:20001) has been deleted
I like it! A few comments. https://codereview.chromium.org/2387733002/diff/1/base/debug/activity_tracker.cc File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/2387733002/diff/1/base/debug/activity_tracker... base/debug/activity_tracker.cc:584: base::AutoLock autolock(thread_tracker_allocator_lock_); Does this recurse? https://codereview.chromium.org/2387733002/diff/1/base/debug/activity_tracker.h File base/debug/activity_tracker.h (right): https://codereview.chromium.org/2387733002/diff/1/base/debug/activity_tracker... base/debug/activity_tracker.h:165: std::unique_ptr<Reference[]> cache_values_; How does a fixed array compare to a vector? Vector gives a variable size, which enables holding more (if needed) to possibly avoid the slow iteration, at the price of potential a reallocation. I have no intuition so I thought I'd ask! ;) https://codereview.chromium.org/2387733002/diff/1/base/debug/activity_tracker... base/debug/activity_tracker.h:505: kCachedThreadMemories = 10, Can you comment on why 10? IIUC caching is cheap compared to having a cache that's too small? 10 is what we expect, plus a buffer? https://codereview.chromium.org/2387733002/diff/1/base/metrics/persistent_mem... File base/metrics/persistent_memory_allocator.h (right): https://codereview.chromium.org/2387733002/diff/1/base/metrics/persistent_mem... base/metrics/persistent_memory_allocator.h:80: void Reset(); Is this dangerous to introduce since we have some "while (GetNext()!=0)" which could lead to infinite looping if a second thread keeps resetting the iterator? Is iterator sharing common? https://codereview.chromium.org/2387733002/diff/1/base/metrics/persistent_mem... base/metrics/persistent_memory_allocator.h:82: // Resets the iterator, resuming from the |starting_after| reference. I'm guessing whenever you Reset(), then GetNext returns the same value? From a quick look, I don't think the comments mentioned the references are ordered (or that the iterator iteration order is). At least it wasn't obvious to me (out of context reader). Add a comment? https://codereview.chromium.org/2387733002/diff/1/base/metrics/persistent_mem... base/metrics/persistent_memory_allocator.h:88: Reference GetLast(); Add a basic test for these?
Another thought: perhaps the cl description could mention this moves from lock free to lock and why.
The CQ bit was checked by bcwhite@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 previous implementation (ThreadSafeStack) wasn't lock-free (sadly). https://codereview.chromium.org/2387733002/diff/1/base/debug/activity_tracker.cc File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/2387733002/diff/1/base/debug/activity_tracker... base/debug/activity_tracker.cc:584: base::AutoLock autolock(thread_tracker_allocator_lock_); On 2016/10/04 21:02:43, manzagop wrote: > Does this recurse? No because lock-tracking isn't allowed to call CreateTrackerForCurrentThread. https://codereview.chromium.org/2387733002/diff/1/base/debug/activity_tracker.h File base/debug/activity_tracker.h (right): https://codereview.chromium.org/2387733002/diff/1/base/debug/activity_tracker... base/debug/activity_tracker.h:165: std::unique_ptr<Reference[]> cache_values_; On 2016/10/04 21:02:43, manzagop wrote: > How does a fixed array compare to a vector? Vector gives a variable size, which > enables holding more (if needed) to possibly avoid the slow iteration, at the > price of potential a reallocation. I have no intuition so I thought I'd ask! ;) The implementation of std::vector is not strictly defined and so can allocate memory at undefined times. Allocating memory can involve locks. I could use a vector and initially reserve the number of slots I need but this ends up being simpler. https://codereview.chromium.org/2387733002/diff/1/base/debug/activity_tracker... base/debug/activity_tracker.h:505: kCachedThreadMemories = 10, On 2016/10/04 21:02:43, manzagop wrote: > Can you comment on why 10? IIUC caching is cheap compared to having a cache > that's too small? 10 is what we expect, plus a buffer? It's arbitrary. If it's too small (many resources created followed by many resources released followed by many resources created), then the "many" beyond this number will cause a search via iteration for free memory blocks. Generally the resources being tracked are fairly permanent so I don't imagine there to be very much churn. I could create metrics for this but that has its own set of complications that I'm choosing to avoid for the moment. https://codereview.chromium.org/2387733002/diff/1/base/metrics/persistent_mem... File base/metrics/persistent_memory_allocator.h (right): https://codereview.chromium.org/2387733002/diff/1/base/metrics/persistent_mem... base/metrics/persistent_memory_allocator.h:80: void Reset(); On 2016/10/04 21:02:43, manzagop wrote: > Is this dangerous to introduce since we have some "while (GetNext()!=0)" which > could lead to infinite looping if a second thread keeps resetting the iterator? > Is iterator sharing common? Each thread has its own iterator and all iterators are completely independent. https://codereview.chromium.org/2387733002/diff/1/base/metrics/persistent_mem... base/metrics/persistent_memory_allocator.h:82: // Resets the iterator, resuming from the |starting_after| reference. On 2016/10/04 21:02:43, manzagop wrote: > I'm guessing whenever you Reset(), then GetNext returns the same value? Yes, the first iterable in the allocator. > From a quick look, I don't think the comments mentioned the references are > ordered (or that the iterator iteration order is). At least it wasn't obvious to > me (out of context reader). Add a comment? Done. https://codereview.chromium.org/2387733002/diff/1/base/metrics/persistent_mem... base/metrics/persistent_memory_allocator.h:88: Reference GetLast(); On 2016/10/04 21:02:43, manzagop wrote: > Add a basic test for these? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
+Alexei, for changes to Iterator in PersistentMemoryAllocator.
The CQ bit was checked by bcwhite@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.
bcwhite@chromium.org changed reviewers: + asvitkine@chromium.org
+asvitkine for real this time.
Patchset #3 (id:60001) has been deleted
https://codereview.chromium.org/2387733002/diff/40001/base/debug/activity_tra... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/2387733002/diff/40001/base/debug/activity_tra... base/debug/activity_tracker.cc:109: if (found && type == ~object_type_) { I don't like you inlining this ~ logic in multiple places. Can you make a helper function? This way, if we want to change it, it can be done once - and the function name clearly spells out what the intention is. https://codereview.chromium.org/2387733002/diff/40001/base/debug/activity_tra... base/debug/activity_tracker.cc:143: // Add this reference to our "free" cache if there is space. Nit: Can you expand comment to mention what the implications of not adding it is? https://codereview.chromium.org/2387733002/diff/40001/base/debug/activity_tra... File base/debug/activity_tracker.h (right): https://codereview.chromium.org/2387733002/diff/40001/base/debug/activity_tra... base/debug/activity_tracker.h:153: size_t cache_used() { return cache_used_; } Nit: const
The CQ bit was checked by bcwhite@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 bcwhite@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...
Patchset #3 (id:80001) has been deleted
https://codereview.chromium.org/2387733002/diff/40001/base/debug/activity_tra... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/2387733002/diff/40001/base/debug/activity_tra... base/debug/activity_tracker.cc:109: if (found && type == ~object_type_) { On 2016/10/13 15:25:21, Alexei Svitkine (slow) wrote: > I don't like you inlining this ~ logic in multiple places. Can you make a helper > function? This way, if we want to change it, it can be done once - and the > function name clearly spells out what the intention is. Done. https://codereview.chromium.org/2387733002/diff/40001/base/debug/activity_tra... base/debug/activity_tracker.cc:143: // Add this reference to our "free" cache if there is space. On 2016/10/13 15:25:21, Alexei Svitkine (slow) wrote: > Nit: Can you expand comment to mention what the implications of not adding it > is? Done. https://codereview.chromium.org/2387733002/diff/40001/base/debug/activity_tra... File base/debug/activity_tracker.h (right): https://codereview.chromium.org/2387733002/diff/40001/base/debug/activity_tra... base/debug/activity_tracker.h:153: size_t cache_used() { return cache_used_; } On 2016/10/13 15:25:21, Alexei Svitkine (slow) wrote: > Nit: const Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2387733002/diff/100001/base/debug/activity_an... File base/debug/activity_analyzer.cc (right): https://codereview.chromium.org/2387733002/diff/100001/base/debug/activity_an... base/debug/activity_analyzer.cc:93: case ~GlobalActivityTracker::kTypeIdActivityTracker: Nit: Can this also use the helper function? You can probably change the switch to an if in that case. Or make another helper in GlobalActivityTracker that asks if it's of that type and inline the logic there, so the details are contained in that code. https://codereview.chromium.org/2387733002/diff/100001/base/debug/activity_tr... File base/debug/activity_tracker.h (right): https://codereview.chromium.org/2387733002/diff/100001/base/debug/activity_tr... base/debug/activity_tracker.h:157: static uint32_t FreeTypeOf(uint32_t object_type) { If you only reference this in the .cc file, put it in the anon namespace.
https://codereview.chromium.org/2387733002/diff/100001/base/debug/activity_an... File base/debug/activity_analyzer.cc (right): https://codereview.chromium.org/2387733002/diff/100001/base/debug/activity_an... base/debug/activity_analyzer.cc:93: case ~GlobalActivityTracker::kTypeIdActivityTracker: On 2016/10/13 19:23:20, Alexei Svitkine (slow) wrote: > Nit: Can this also use the helper function? You can probably change the switch > to an if in that case. > > Or make another helper in GlobalActivityTracker that asks if it's of that type > and inline the logic there, so the details are contained in that code. How about if I just make the class take a "free type" constructor parameter, instead? The free type can be defined in the enum as anything (including free_type=~type). Helper methods go away and everything is clean again. Cost is one more const uint32_t member variable.
Sure, that sounds good. On Thu, Oct 13, 2016 at 3:35 PM, <bcwhite@chromium.org> wrote: > > https://codereview.chromium.org/2387733002/diff/100001/ > base/debug/activity_analyzer.cc > File base/debug/activity_analyzer.cc (right): > > https://codereview.chromium.org/2387733002/diff/100001/ > base/debug/activity_analyzer.cc#newcode93 > base/debug/activity_analyzer.cc:93: case > ~GlobalActivityTracker::kTypeIdActivityTracker: > On 2016/10/13 19:23:20, Alexei Svitkine (slow) wrote: > > Nit: Can this also use the helper function? You can probably change > the switch > > to an if in that case. > > > > Or make another helper in GlobalActivityTracker that asks if it's of > that type > > and inline the logic there, so the details are contained in that code. > > How about if I just make the class take a "free type" constructor > parameter, instead? The free type can be defined in the enum as > anything (including free_type=~type). Helper methods go away and > everything is clean again. Cost is one more const uint32_t member > variable. > > https://codereview.chromium.org/2387733002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by bcwhite@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 bcwhite@chromium.org to run a CQ dry run
Patchset #4 (id:120001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Redone with a "free" type ctor-parameter.
lgtm https://codereview.chromium.org/2387733002/diff/140001/base/debug/activity_tr... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/2387733002/diff/140001/base/debug/activity_tr... base/debug/activity_tracker.cc:72: uint32_t object_free, Nit: object_free_type so that "type" is still part of the name.
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 bcwhite@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...
Patchset #5 (id:160001) has been deleted
https://codereview.chromium.org/2387733002/diff/140001/base/debug/activity_tr... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/2387733002/diff/140001/base/debug/activity_tr... base/debug/activity_tracker.cc:72: uint32_t object_free, On 2016/10/13 20:41:15, Alexei Svitkine (slow) wrote: > Nit: object_free_type so that "type" is still part of the name. Done.
The CQ bit was checked by bcwhite@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 bcwhite@chromium.org
The CQ bit was checked by bcwhite@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from manzagop@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2387733002/#ps180001 (title: "object_free -> object_free_type")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #5 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Move memory management code into separate class for future reuse. BUG=620813 ========== to ========== Move memory management code into separate class for future reuse. BUG=620813 Committed: https://crrev.com/dadd3155cd102736da470fc28fc5aae89c7f0ac3 Cr-Commit-Position: refs/heads/master@{#425206} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/dadd3155cd102736da470fc28fc5aae89c7f0ac3 Cr-Commit-Position: refs/heads/master@{#425206} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
