Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2806403002:
Support delayed allocations from persistent memory. (Closed)
|
Created:
3 years, 8 months ago by bcwhite Modified:
3 years, 8 months ago Reviewers:
Alexei Svitkine (slow) CC:
chromium-reviews, asvitkine+watch_chromium.org, vmpstr+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionSupport delayed allocations from persistent memory.
This allows for an allocation to be defined by code that knows about
persistent allocation but not be realized until more generic code
actually needs the space.
In addition, delayed allocations can be split and shared such that once
its needed in one place it will be available in all places, an all-or-
nothing arrangement.
This is done in support of follow-up CL:
https://codereview.chromium.org/2811713003/
BUG=705342
Review-Url: https://codereview.chromium.org/2806403002
Cr-Commit-Position: refs/heads/master@{#466374}
Committed: https://chromium.googlesource.com/chromium/src/+/1166f8da78cfb4071f088e0f85fd940429d785fe
Patch Set 1 #Patch Set 2 : fixed build problems #
Total comments: 24
Patch Set 3 : addressed review comments by asvitkine #Patch Set 4 : addressed review comments by asvitkine #
Dependent Patchsets: Messages
Total messages: 30 (21 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: 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_...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) 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 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.
Description was changed from ========== Support delayed allocations from persistent memory. This allows for an allocation to be defined by code that knows about persistent allocation but not be realized until more generic code actually needs the space. In addition, delayed allocations can be split and shared such that once its needed in one place it will be available in all places, an all-or- nothing arrangement. BUG=705342 ========== to ========== Support delayed allocations from persistent memory. This allows for an allocation to be defined by code that knows about persistent allocation but not be realized until more generic code actually needs the space. In addition, delayed allocations can be split and shared such that once its needed in one place it will be available in all places, an all-or- nothing arrangement. This is done in support of follow-up CL: https://codereview.chromium.org/2811713003/ BUG=705342 ==========
bcwhite@chromium.org changed reviewers: + asvitkine@chromium.org
Sorry, I've not had a chance to get to this or the other CL yet. :\ Was in meetings most of yesterday and today - planning to hopefully get to these tomorrow.
Some initial comments. Haven't looked at the other CL that uses it first. https://codereview.chromium.org/2806403002/diff/20001/base/metrics/persistent... File base/metrics/persistent_memory_allocator.cc (right): https://codereview.chromium.org/2806403002/diff/20001/base/metrics/persistent... base/metrics/persistent_memory_allocator.cc:1079: 0) {} Nit: Add empty lines between ctors to make it easier for a human to parse. https://codereview.chromium.org/2806403002/diff/20001/base/metrics/persistent... base/metrics/persistent_memory_allocator.cc:1110: reference_(ref) {} Nit: Add a DCHECK() on the ref ptr not being null? https://codereview.chromium.org/2806403002/diff/20001/base/metrics/persistent... base/metrics/persistent_memory_allocator.cc:1115: make_iterable_ = true; How about the case where it was created already and MakeIterable() is called after that? Either we should not support that explicitly (and add a DCHECK here that Get() hasn't been called yet) and document it - or this should do more work if there's already a ref set. I think the former (not supporting it explicitly) is fine given it doesn't seem we need it. https://codereview.chromium.org/2806403002/diff/20001/base/metrics/persistent... base/metrics/persistent_memory_allocator.cc:1119: // Relaxed operations is acceptable here because it's not protecting the Nit: is -> are https://codereview.chromium.org/2806403002/diff/20001/base/metrics/persistent... base/metrics/persistent_memory_allocator.cc:1138: // allocation, and stored its reference. Purge the allocation just done Nit: "that was just done" as otherwise it's a bit hard to read. https://codereview.chromium.org/2806403002/diff/20001/base/metrics/persistent... File base/metrics/persistent_memory_allocator.h (right): https://codereview.chromium.org/2806403002/diff/20001/base/metrics/persistent... base/metrics/persistent_memory_allocator.h:785: // |size|. If |offset| is given, the returned pointer will be at that Please expand comment to make explicit whether |size| includes offset or doesn't. https://codereview.chromium.org/2806403002/diff/20001/base/metrics/persistent... base/metrics/persistent_memory_allocator.h:787: // single persistent segment to reduce overhead and means as "all or Nit: "means as" -> "means an" https://codereview.chromium.org/2806403002/diff/20001/base/metrics/persistent... base/metrics/persistent_memory_allocator.h:795: // For conveience, methods taking both Atomic32 and std::atomic<Reference> Nit: convenience https://codereview.chromium.org/2806403002/diff/20001/base/metrics/persistent... base/metrics/persistent_memory_allocator.h:846: uint32_t const type_; Nit: I think for primitive types, it's more common to put const at the front. Let's use that style for consistency with more code. https://codereview.chromium.org/2806403002/diff/20001/base/metrics/persistent... base/metrics/persistent_memory_allocator.h:858: }; DISALLOW_COPY_AND_ASSIGN()?
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...
https://codereview.chromium.org/2806403002/diff/20001/base/metrics/persistent... File base/metrics/persistent_memory_allocator.cc (right): https://codereview.chromium.org/2806403002/diff/20001/base/metrics/persistent... base/metrics/persistent_memory_allocator.cc:1079: 0) {} On 2017/04/12 17:04:24, Alexei Svitkine (very slow) wrote: > Nit: Add empty lines between ctors to make it easier for a human to parse. Done. https://codereview.chromium.org/2806403002/diff/20001/base/metrics/persistent... base/metrics/persistent_memory_allocator.cc:1110: reference_(ref) {} On 2017/04/12 17:04:25, Alexei Svitkine (very slow) wrote: > Nit: Add a DCHECK() on the ref ptr not being null? Done. https://codereview.chromium.org/2806403002/diff/20001/base/metrics/persistent... base/metrics/persistent_memory_allocator.cc:1115: make_iterable_ = true; On 2017/04/12 17:04:24, Alexei Svitkine (very slow) wrote: > How about the case where it was created already and MakeIterable() is called > after that? > > Either we should not support that explicitly (and add a DCHECK here that Get() > hasn't been called yet) and document it - or this should do more work if there's > already a ref set. I think the former (not supporting it explicitly) is fine > given it doesn't seem we need it. It would be called when the object is created and not by whatever is doing Get() so it will happen first. It could be a ctor parameter. https://codereview.chromium.org/2806403002/diff/20001/base/metrics/persistent... base/metrics/persistent_memory_allocator.cc:1119: // Relaxed operations is acceptable here because it's not protecting the On 2017/04/12 17:04:25, Alexei Svitkine (very slow) wrote: > Nit: is -> are Done. https://codereview.chromium.org/2806403002/diff/20001/base/metrics/persistent... base/metrics/persistent_memory_allocator.cc:1138: // allocation, and stored its reference. Purge the allocation just done On 2017/04/12 17:04:25, Alexei Svitkine (very slow) wrote: > Nit: "that was just done" as otherwise it's a bit hard to read. Done. https://codereview.chromium.org/2806403002/diff/20001/base/metrics/persistent... File base/metrics/persistent_memory_allocator.h (right): https://codereview.chromium.org/2806403002/diff/20001/base/metrics/persistent... base/metrics/persistent_memory_allocator.h:785: // |size|. If |offset| is given, the returned pointer will be at that On 2017/04/12 17:04:25, Alexei Svitkine (very slow) wrote: > Please expand comment to make explicit whether |size| includes offset or > doesn't. Done. https://codereview.chromium.org/2806403002/diff/20001/base/metrics/persistent... base/metrics/persistent_memory_allocator.h:787: // single persistent segment to reduce overhead and means as "all or On 2017/04/12 17:04:25, Alexei Svitkine (very slow) wrote: > Nit: "means as" -> "means an" Done. https://codereview.chromium.org/2806403002/diff/20001/base/metrics/persistent... base/metrics/persistent_memory_allocator.h:795: // For conveience, methods taking both Atomic32 and std::atomic<Reference> On 2017/04/12 17:04:25, Alexei Svitkine (very slow) wrote: > Nit: convenience Done. https://codereview.chromium.org/2806403002/diff/20001/base/metrics/persistent... base/metrics/persistent_memory_allocator.h:846: uint32_t const type_; On 2017/04/12 17:04:25, Alexei Svitkine (very slow) wrote: > Nit: I think for primitive types, it's more common to put const at the front. > Let's use that style for consistency with more code. Done. https://codereview.chromium.org/2806403002/diff/20001/base/metrics/persistent... base/metrics/persistent_memory_allocator.h:858: }; On 2017/04/12 17:04:25, Alexei Svitkine (very slow) wrote: > DISALLOW_COPY_AND_ASSIGN()? No, it's explicitly allowed and safe. This makes it easy to pass it as a reference and copy it to the final destination.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2806403002/diff/20001/base/metrics/persistent... File base/metrics/persistent_memory_allocator.cc (right): https://codereview.chromium.org/2806403002/diff/20001/base/metrics/persistent... base/metrics/persistent_memory_allocator.cc:1115: make_iterable_ = true; On 2017/04/12 21:23:42, bcwhite wrote: > On 2017/04/12 17:04:24, Alexei Svitkine (very slow) wrote: > > How about the case where it was created already and MakeIterable() is called > > after that? > > > > Either we should not support that explicitly (and add a DCHECK here that Get() > > hasn't been called yet) and document it - or this should do more work if > there's > > already a ref set. I think the former (not supporting it explicitly) is fine > > given it doesn't seem we need it. > > It would be called when the object is created and not by whatever is doing Get() > so it will happen first. It could be a ctor parameter. Let's make it a ctor param them - to avoid confusion about it being possible to call it later. However, looking again - it doesn't seem like you use it from the other CL. If so, how about just removing it to simplify the code? Or is it needed for the tests you have? If that's the case, then call it MakeIterableForTesting(). (If later there's a use case for it to be called outside of tests, then it can be renamed.) https://codereview.chromium.org/2806403002/diff/20001/base/metrics/persistent... File base/metrics/persistent_memory_allocator.h (right): https://codereview.chromium.org/2806403002/diff/20001/base/metrics/persistent... base/metrics/persistent_memory_allocator.h:858: }; On 2017/04/12 21:23:42, bcwhite wrote: > On 2017/04/12 17:04:25, Alexei Svitkine (very slow) wrote: > > DISALLOW_COPY_AND_ASSIGN()? > > No, it's explicitly allowed and safe. This makes it easy to pass it as a > reference and copy it to the final destination. Okay, add a comment about it then in the same place where that macro would be. :)
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...
https://codereview.chromium.org/2806403002/diff/20001/base/metrics/persistent... File base/metrics/persistent_memory_allocator.cc (right): https://codereview.chromium.org/2806403002/diff/20001/base/metrics/persistent... base/metrics/persistent_memory_allocator.cc:1115: make_iterable_ = true; On 2017/04/21 15:14:16, Alexei Svitkine (slow) wrote: > On 2017/04/12 21:23:42, bcwhite wrote: > > On 2017/04/12 17:04:24, Alexei Svitkine (very slow) wrote: > > > How about the case where it was created already and MakeIterable() is called > > > after that? > > > > > > Either we should not support that explicitly (and add a DCHECK here that > Get() > > > hasn't been called yet) and document it - or this should do more work if > > there's > > > already a ref set. I think the former (not supporting it explicitly) is fine > > > given it doesn't seem we need it. > > > > It would be called when the object is created and not by whatever is doing > Get() > > so it will happen first. It could be a ctor parameter. > > Let's make it a ctor param them - to avoid confusion about it being possible to > call it later. > > However, looking again - it doesn't seem like you use it from the other CL. If > so, how about just removing it to simplify the code? Or is it needed for the > tests you have? If that's the case, then call it MakeIterableForTesting(). > > (If later there's a use case for it to be called outside of tests, then it can > be renamed.) I like it better as a ctor parameter. Less confusion as the method sounds like it should operate immediately when it does not. https://codereview.chromium.org/2806403002/diff/20001/base/metrics/persistent... File base/metrics/persistent_memory_allocator.h (right): https://codereview.chromium.org/2806403002/diff/20001/base/metrics/persistent... base/metrics/persistent_memory_allocator.h:858: }; On 2017/04/21 15:14:16, Alexei Svitkine (slow) wrote: > On 2017/04/12 21:23:42, bcwhite wrote: > > On 2017/04/12 17:04:25, Alexei Svitkine (very slow) wrote: > > > DISALLOW_COPY_AND_ASSIGN()? > > > > No, it's explicitly allowed and safe. This makes it easy to pass it as a > > reference and copy it to the final destination. > > Okay, add a comment about it then in the same place where that macro would be. > :) Done.
lgtm
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
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1492794888096920,
"parent_rev": "889c9f9d19ed88f19a52149f7b4afe75d68feb17", "commit_rev":
"1166f8da78cfb4071f088e0f85fd940429d785fe"}
Message was sent while issue was closed.
Description was changed from ========== Support delayed allocations from persistent memory. This allows for an allocation to be defined by code that knows about persistent allocation but not be realized until more generic code actually needs the space. In addition, delayed allocations can be split and shared such that once its needed in one place it will be available in all places, an all-or- nothing arrangement. This is done in support of follow-up CL: https://codereview.chromium.org/2811713003/ BUG=705342 ========== to ========== Support delayed allocations from persistent memory. This allows for an allocation to be defined by code that knows about persistent allocation but not be realized until more generic code actually needs the space. In addition, delayed allocations can be split and shared such that once its needed in one place it will be available in all places, an all-or- nothing arrangement. This is done in support of follow-up CL: https://codereview.chromium.org/2811713003/ BUG=705342 Review-Url: https://codereview.chromium.org/2806403002 Cr-Commit-Position: refs/heads/master@{#466374} Committed: https://chromium.googlesource.com/chromium/src/+/1166f8da78cfb4071f088e0f85fd... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/1166f8da78cfb4071f088e0f85fd... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
