Chromium Code Reviews
|
|
|
Created:
4 years, 10 months ago by Chris Evans Modified:
4 years, 10 months ago Reviewers:
haraken CC:
blink-reviews, Mads Ager (chromium), oilpan-reviews, blink-reviews-wtf_chromium.org, kouhei+heap_chromium.org, Mikhail Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionPartitionAlloc: improve clarity of page state checks.
No functional changes. This CL introduces functions to check for the four
different page stages, and uses them consistently. It improves code clarity
and tightens ASSERTing a little.
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197701
Patch Set 1 #
Total comments: 3
Patch Set 2 : Review feedback. #Messages
Total messages: 12 (5 generated)
cevans@chromium.org changed reviewers: + haraken@chromium.org
LGTM https://codereview.chromium.org/1201303002/diff/1/Source/wtf/PartitionAlloc.cpp File Source/wtf/PartitionAlloc.cpp (right): https://codereview.chromium.org/1201303002/diff/1/Source/wtf/PartitionAlloc.c... Source/wtf/PartitionAlloc.cpp:328: return (page->numAllocatedSlots && (page->freelistHead || page->numUnprovisionedSlots)); page->numAllocatedSlots => page->numAllocatedSlots > 0 Just to confirm: If page->numAllocatedSlots > 0 but 'page->freelistHead || page->numUnprovisionedSlots' is false, it is a full page, right? https://codereview.chromium.org/1201303002/diff/1/Source/wtf/PartitionAlloc.c... Source/wtf/PartitionAlloc.cpp:331: static bool ALWAYS_INLINE partitionPageStateIsFull(const PartitionPage* page) This method is a bit confusing, since it returns false for a full page that has already been removed from the active page list. I'm OK in this CL, but it would be great if we can clean up the page state transitions a bit more in the future. For example, it looks hacky to represent a full page by storing a negative value in numAllocatedSlots. struct PartitionPage { ...; uint16_t pageOffset; // We won't need 16 bit for this. int16_t emptyCacheIndex; // Ditto. }; By saving the bits, we'll be able to have space to explicitly represent the PageState. https://codereview.chromium.org/1201303002/diff/1/Source/wtf/PartitionAlloc.c... Source/wtf/PartitionAlloc.cpp:865: page->numUnprovisionedSlots = 0; Shall we add ASSERT(partitionPageStateIsDecommitted(page)) ?
On 2015/06/24 00:51:52, haraken wrote: > LGTM > > https://codereview.chromium.org/1201303002/diff/1/Source/wtf/PartitionAlloc.cpp > File Source/wtf/PartitionAlloc.cpp (right): > > https://codereview.chromium.org/1201303002/diff/1/Source/wtf/PartitionAlloc.c... > Source/wtf/PartitionAlloc.cpp:328: return (page->numAllocatedSlots && > (page->freelistHead || page->numUnprovisionedSlots)); > > page->numAllocatedSlots => page->numAllocatedSlots > 0 Done. Also added ASSERT(page->numAllocatedSlots >= 0) to all cases. It's only valid to call these functions on pages found in one of the page lists. > > Just to confirm: If page->numAllocatedSlots > 0 but 'page->freelistHead || > page->numUnprovisionedSlots' is false, it is a full page, right? Yep. > > https://codereview.chromium.org/1201303002/diff/1/Source/wtf/PartitionAlloc.c... > Source/wtf/PartitionAlloc.cpp:331: static bool ALWAYS_INLINE > partitionPageStateIsFull(const PartitionPage* page) > > This method is a bit confusing, since it returns false for a full page that has > already been removed from the active page list. Yeah, it's not valid to call these functions on a full page that got yanked out of all the list. I added a comment. > > I'm OK in this CL, but it would be great if we can clean up the page state > transitions a bit more in the future. For example, it looks hacky to represent a > full page by storing a negative value in numAllocatedSlots. Ah, that trick is what enables us to have a single-branch-hot-path for free(), which I believe is one of the reasons PartitionAlloc is so fast! It'd be a shame to lose that. > > struct PartitionPage { > ...; > uint16_t pageOffset; // We won't need 16 bit for this. > int16_t emptyCacheIndex; // Ditto. > }; > > By saving the bits, we'll be able to have space to explicitly represent the > PageState. I'll think about this, specifically whether it can be done without slowing down the fast path. For example, if a page on the active list goes from full to not full, we currently don't enter the slow path for that. Having a couple of extra filds in PartitionPage is exciting, though! Can we think of extra metadata to store that would either save us memory or speed things up? > > https://codereview.chromium.org/1201303002/diff/1/Source/wtf/PartitionAlloc.c... > Source/wtf/PartitionAlloc.cpp:865: page->numUnprovisionedSlots = 0; > > Shall we add ASSERT(partitionPageStateIsDecommitted(page)) ? Done.
On 2015/06/24 02:56:53, Chris Evans wrote: > On 2015/06/24 00:51:52, haraken wrote: > > LGTM > > > > > https://codereview.chromium.org/1201303002/diff/1/Source/wtf/PartitionAlloc.cpp > > File Source/wtf/PartitionAlloc.cpp (right): > > > > > https://codereview.chromium.org/1201303002/diff/1/Source/wtf/PartitionAlloc.c... > > Source/wtf/PartitionAlloc.cpp:328: return (page->numAllocatedSlots && > > (page->freelistHead || page->numUnprovisionedSlots)); > > > > page->numAllocatedSlots => page->numAllocatedSlots > 0 > > Done. Also added ASSERT(page->numAllocatedSlots >= 0) to all cases. It's only > valid to call these functions on pages found in one of the page lists. Actually, didn't add the ASSERT for now because "detached full pages" can occur on the globalEmptyPageRing. > > > > > Just to confirm: If page->numAllocatedSlots > 0 but 'page->freelistHead || > > page->numUnprovisionedSlots' is false, it is a full page, right? > > Yep. > > > > > > https://codereview.chromium.org/1201303002/diff/1/Source/wtf/PartitionAlloc.c... > > Source/wtf/PartitionAlloc.cpp:331: static bool ALWAYS_INLINE > > partitionPageStateIsFull(const PartitionPage* page) > > > > This method is a bit confusing, since it returns false for a full page that > has > > already been removed from the active page list. > > Yeah, it's not valid to call these functions on a full page that got yanked out > of all the list. I added a comment. > > > > > I'm OK in this CL, but it would be great if we can clean up the page state > > transitions a bit more in the future. For example, it looks hacky to represent > a > > full page by storing a negative value in numAllocatedSlots. > > Ah, that trick is what enables us to have a single-branch-hot-path for free(), > which I believe is one of the reasons PartitionAlloc is so fast! It'd be a shame > to lose that. > > > > > struct PartitionPage { > > ...; > > uint16_t pageOffset; // We won't need 16 bit for this. > > int16_t emptyCacheIndex; // Ditto. > > }; > > > > By saving the bits, we'll be able to have space to explicitly represent the > > PageState. > > I'll think about this, specifically whether it can be done without slowing down > the fast path. For example, if a page on the active list goes from full to not > full, we currently don't enter the slow path for that. > > Having a couple of extra filds in PartitionPage is exciting, though! Can we > think of extra metadata to store that would either save us memory or speed > things up? > > > > > > https://codereview.chromium.org/1201303002/diff/1/Source/wtf/PartitionAlloc.c... > > Source/wtf/PartitionAlloc.cpp:865: page->numUnprovisionedSlots = 0; > > > > Shall we add ASSERT(partitionPageStateIsDecommitted(page)) ? > > Done.
The CQ bit was checked by cevans@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org Link to the patchset: https://codereview.chromium.org/1201303002/#ps20001 (title: "Review feedback.")
The CQ bit was unchecked by cevans@chromium.org
The CQ bit was checked by cevans@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1201303002/20001
On 2015/06/24 03:05:20, Chris Evans wrote: > On 2015/06/24 02:56:53, Chris Evans wrote: > > On 2015/06/24 00:51:52, haraken wrote: > > > LGTM > > > > > > > > > https://codereview.chromium.org/1201303002/diff/1/Source/wtf/PartitionAlloc.cpp > > > File Source/wtf/PartitionAlloc.cpp (right): > > > > > > > > > https://codereview.chromium.org/1201303002/diff/1/Source/wtf/PartitionAlloc.c... > > > Source/wtf/PartitionAlloc.cpp:328: return (page->numAllocatedSlots && > > > (page->freelistHead || page->numUnprovisionedSlots)); > > > > > > page->numAllocatedSlots => page->numAllocatedSlots > 0 > > > > Done. Also added ASSERT(page->numAllocatedSlots >= 0) to all cases. It's only > > valid to call these functions on pages found in one of the page lists. > > Actually, didn't add the ASSERT for now because "detached full pages" can occur > on the globalEmptyPageRing. > > > > > > > > > Just to confirm: If page->numAllocatedSlots > 0 but 'page->freelistHead || > > > page->numUnprovisionedSlots' is false, it is a full page, right? > > > > Yep. > > > > > > > > > > > https://codereview.chromium.org/1201303002/diff/1/Source/wtf/PartitionAlloc.c... > > > Source/wtf/PartitionAlloc.cpp:331: static bool ALWAYS_INLINE > > > partitionPageStateIsFull(const PartitionPage* page) > > > > > > This method is a bit confusing, since it returns false for a full page that > > has > > > already been removed from the active page list. > > > > Yeah, it's not valid to call these functions on a full page that got yanked > out > > of all the list. I added a comment. > > > > > > > > I'm OK in this CL, but it would be great if we can clean up the page state > > > transitions a bit more in the future. For example, it looks hacky to > represent > > a > > > full page by storing a negative value in numAllocatedSlots. > > > > Ah, that trick is what enables us to have a single-branch-hot-path for free(), > > which I believe is one of the reasons PartitionAlloc is so fast! It'd be a > shame > > to lose that. > > > > > > > > struct PartitionPage { > > > ...; > > > uint16_t pageOffset; // We won't need 16 bit for this. > > > int16_t emptyCacheIndex; // Ditto. > > > }; > > > > > > By saving the bits, we'll be able to have space to explicitly represent the > > > PageState. > > > > I'll think about this, specifically whether it can be done without slowing > down > > the fast path. For example, if a page on the active list goes from full to not > > full, we currently don't enter the slow path for that. Ah, this makes sense :) > > Having a couple of extra filds in PartitionPage is exciting, though! Can we > > think of extra metadata to store that would either save us memory or speed > > things up? Maybe we can store something like # of freed slots, # of not-discarded pages etc to avoid doing a full scan of pages that don't have discardable system pages?
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://src.chromium.org/viewvc/blink?view=rev&revision=197701 |
