Chromium Code Reviews
|
|
|
Created:
4 years, 10 months ago by Chris Evans Modified:
4 years, 10 months ago 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@rawsize Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionPartitionAlloc: implement discarding for partitionPurgeMemoryGeneric().
There are two instances where we can return memory to the system, above and
beyond purging the free page list:
- Unused system pages at the end of larger allocations.
- Unused slots within slot spans for >4096 byte buckets.
We now have all the metadata and support APIs necessary to discard these unused
pages, so add a new flag to partitionPurgeMemoryGeneric() to support this,
along with tests.
BUG=501878
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197626
Patch Set 1 #
Total comments: 7
Patch Set 2 : Action review comments. #Patch Set 3 : Add missing comment. #
Total comments: 7
Patch Set 4 : Action review feedback. #
Total comments: 1
Patch Set 5 : Fix Windows compile error. #
Messages
Total messages: 17 (6 generated)
cevans@chromium.org changed reviewers: + haraken@chromium.org
https://codereview.chromium.org/1197753003/diff/1/Source/wtf/PartitionAlloc.cpp File Source/wtf/PartitionAlloc.cpp (right): https://codereview.chromium.org/1197753003/diff/1/Source/wtf/PartitionAlloc.c... Source/wtf/PartitionAlloc.cpp:1065: static void partitionPageWalk(PartitionBucketMemoryStats* statsOut, const PartitionPage* page, bool discardMemory) Maybe you can return the discardableBytes from this method. Then this method doesn't need to take the statsOut parameter. (See my comment below for why I want to do this.) https://codereview.chromium.org/1197753003/diff/1/Source/wtf/PartitionAlloc.c... Source/wtf/PartitionAlloc.cpp:1132: ASSERT(((pageBytesResident - activeBytes) % kSystemPageSize) == 0); Just help me understand: How is it guaranteed that activeBytes is a multiple of kSystemPageSize? https://codereview.chromium.org/1197753003/diff/1/Source/wtf/PartitionAlloc.c... Source/wtf/PartitionAlloc.cpp:1138: discardSystemPages(ptr, discardable); I'm a bit afraid that if we have a bug in the discarding algorithm, we will end up with hitting a crash that is hard to investigate. In ASSERT builds, how about zapping the discarded memory instead of really discarding the memory? (We can do this in a follow-up.) https://codereview.chromium.org/1197753003/diff/1/Source/wtf/PartitionAlloc.c... Source/wtf/PartitionAlloc.cpp:1146: ++statsOut->numEmptyPages; Shall we call discardSystemPages here? Given that we call partitionPageWalk and discard memory of active pages, it would make more sense to discard memory of empty pages as well. https://codereview.chromium.org/1197753003/diff/1/Source/wtf/PartitionAlloc.c... Source/wtf/PartitionAlloc.cpp:1211: partitionDecommitEmptyPages(root); The reason you don't call partitionGenericBucketWalk in partitionPurgeMemory is that those partitions don't have a bucket larger than 4096 byte? (Maybe worth a comment.) https://codereview.chromium.org/1197753003/diff/1/Source/wtf/PartitionAlloc.c... Source/wtf/PartitionAlloc.cpp:1220: PartitionBucketMemoryStats bucketStats[kGenericNumBuckets]; It is redundant to prepare an array of dummy bucketStats that won't be used. Also conceptually, I'd prefer separating stats code from actual code. I guess you shared the stats code with the actual code to avoid code duplication, but at a first glance, it seems that we can split the two code without introducing too much duplication. What do you think? https://codereview.chromium.org/1197753003/diff/1/Source/wtf/PartitionAlloc.c... Source/wtf/PartitionAlloc.cpp:1221: partitionGenericBucketWalk(bucketStats, root, true); Or you can pass a nullptr to statsOut and remove the discardMemory flag. We can discard memory if statsOut==nullptr. (But I'd prefer splitting the stats code from the actual code.)
On 2015/06/22 03:36:59, haraken wrote: > https://codereview.chromium.org/1197753003/diff/1/Source/wtf/PartitionAlloc.cpp > File Source/wtf/PartitionAlloc.cpp (right): > > https://codereview.chromium.org/1197753003/diff/1/Source/wtf/PartitionAlloc.c... > Source/wtf/PartitionAlloc.cpp:1065: static void > partitionPageWalk(PartitionBucketMemoryStats* statsOut, const PartitionPage* > page, bool discardMemory) > > Maybe you can return the discardableBytes from this method. Then this method > doesn't need to take the statsOut parameter. (See my comment below for why I > want to do this.) Done. > > https://codereview.chromium.org/1197753003/diff/1/Source/wtf/PartitionAlloc.c... > Source/wtf/PartitionAlloc.cpp:1132: ASSERT(((pageBytesResident - activeBytes) % > kSystemPageSize) == 0); > > Just help me understand: How is it guaranteed that activeBytes is a multiple of > kSystemPageSize? We rounded up both the LHS and RHS to kSystemPageSize just above. > > https://codereview.chromium.org/1197753003/diff/1/Source/wtf/PartitionAlloc.c... > Source/wtf/PartitionAlloc.cpp:1138: discardSystemPages(ptr, discardable); > > I'm a bit afraid that if we have a bug in the discarding algorithm, we will end > up with hitting a crash that is hard to investigate. In ASSERT builds, how about > zapping the discarded memory instead of really discarding the memory? (We can do > this in a follow-up.) I agree -- this is a concern. What do you mean by "zapping"? We could PROT_NONE the range, but then we'd be in trouble because we don't have page-level metadata for which slots are zapped and which are not. We'd find it very hard to write the "unzap" code :) Alternatively, we could memset() the range with some recognizable constant. Or perhaps in DEBUG builds, we could do an auto-purge every N allocations to shake out issues? > > https://codereview.chromium.org/1197753003/diff/1/Source/wtf/PartitionAlloc.c... > Source/wtf/PartitionAlloc.cpp:1146: ++statsOut->numEmptyPages; > > Shall we call discardSystemPages here? Given that we call partitionPageWalk and > discard memory of active pages, it would make more sense to discard memory of > empty pages as well. Discarding the memory of empty pages is handled by a different flag (PartitionPurgeDecommitEmptyPages). > > https://codereview.chromium.org/1197753003/diff/1/Source/wtf/PartitionAlloc.c... > Source/wtf/PartitionAlloc.cpp:1211: partitionDecommitEmptyPages(root); > > The reason you don't call partitionGenericBucketWalk in partitionPurgeMemory is > that those partitions don't have a bucket larger than 4096 byte? (Maybe worth a > comment.) Yep. Comment added. > > https://codereview.chromium.org/1197753003/diff/1/Source/wtf/PartitionAlloc.c... > Source/wtf/PartitionAlloc.cpp:1220: PartitionBucketMemoryStats > bucketStats[kGenericNumBuckets]; > > It is redundant to prepare an array of dummy bucketStats that won't be used. > Also conceptually, I'd prefer separating stats code from actual code. I guess > you shared the stats code with the actual code to avoid code duplication, but at > a first glance, it seems that we can split the two code without introducing too > much duplication. What do you think? You're right, I split the two out and it looks better. > > https://codereview.chromium.org/1197753003/diff/1/Source/wtf/PartitionAlloc.c... > Source/wtf/PartitionAlloc.cpp:1221: partitionGenericBucketWalk(bucketStats, > root, true); > > Or you can pass a nullptr to statsOut and remove the discardMemory flag. We can > discard memory if statsOut==nullptr. > > (But I'd prefer splitting the stats code from the actual code.)
> https://codereview.chromium.org/1197753003/diff/1/Source/wtf/PartitionAlloc.c... > > Source/wtf/PartitionAlloc.cpp:1138: discardSystemPages(ptr, discardable); > > > > I'm a bit afraid that if we have a bug in the discarding algorithm, we will > end > > up with hitting a crash that is hard to investigate. In ASSERT builds, how > about > > zapping the discarded memory instead of really discarding the memory? (We can > do > > this in a follow-up.) > > I agree -- this is a concern. > What do you mean by "zapping"? We could PROT_NONE the range, but then we'd be in > trouble because we don't have page-level metadata for which slots are zapped and > which are not. We'd find it very hard to write the "unzap" code :) > Alternatively, we could memset() the range with some recognizable constant. > Or perhaps in DEBUG builds, we could do an auto-purge every N allocations to > shake out issues? By "zapping", I mean memsetting the range with some recognizable constant :) (Taking another look.)
Mostly looks good; a final round of comments :) https://codereview.chromium.org/1197753003/diff/40001/Source/wtf/PartitionAll... File Source/wtf/PartitionAlloc.cpp (right): https://codereview.chromium.org/1197753003/diff/40001/Source/wtf/PartitionAll... Source/wtf/PartitionAlloc.cpp:1056: void partitionPurgeMemory(PartitionRoot* root, int flags) I'd move this to just before partitionPurgeMemoryGeneric. https://codereview.chromium.org/1197753003/diff/40001/Source/wtf/PartitionAll... Source/wtf/PartitionAlloc.cpp:1078: discardableBytes = bucket->slotSize - usedBytes; Is it guaranteed that bucket->slotSize is a multiple of the system page size? https://codereview.chromium.org/1197753003/diff/40001/Source/wtf/PartitionAll... Source/wtf/PartitionAlloc.cpp:1082: discardSystemPages(ptr, discardableBytes); For a single-slot allocation, we're committing the "trailing" system pages when we allocated the page, and then discarding the "trailing" system pages here. This looks a bit redundant. Can we avoid committing the "trailing" system pages from the beginning? (We can do this in a follow-up.) https://codereview.chromium.org/1197753003/diff/40001/Source/wtf/PartitionAll... Source/wtf/PartitionAlloc.cpp:1087: char slotUsage[(kPartitionPageSize * kMaxPartitionPagesPerSlotSpan) / kSystemPageSize]; size_t maxSlotCount = (kPartitionPageSize * kMaxPartitionPagesPerSlotSpan) / kSystemPageSize; char slotUsage[maxSlotCount]; ... and then add ASSERT(slotIndex < maxSlotCount) when accessing the slotUsage array. https://codereview.chromium.org/1197753003/diff/40001/Source/wtf/PartitionAll... Source/wtf/PartitionAlloc.cpp:1091: PartitionFreelistEntry* fl = page->freelistHead; fl => entry or freelist https://codereview.chromium.org/1197753003/diff/40001/Source/wtf/PartitionAll... Source/wtf/PartitionAlloc.cpp:1099: if (!fl && !partitionFreelistMask(fl)) Is it possible that this condition becomes true? I guess fl and partitionFreelistMask(fl) don't become 0 at the same time. To avoid the complexity, I might prefer just dropping the lastSlot optimization :) https://codereview.chromium.org/1197753003/diff/40001/Source/wtf/PartitionAll... Source/wtf/PartitionAlloc.cpp:1136: (void) partitionPurgePage(page, true); Nit: (void) won't be needed.
On 2015/06/22 09:22:00, haraken wrote: > Mostly looks good; a final round of comments :) > > https://codereview.chromium.org/1197753003/diff/40001/Source/wtf/PartitionAll... > File Source/wtf/PartitionAlloc.cpp (right): > > https://codereview.chromium.org/1197753003/diff/40001/Source/wtf/PartitionAll... > Source/wtf/PartitionAlloc.cpp:1056: void partitionPurgeMemory(PartitionRoot* > root, int flags) > > I'd move this to just before partitionPurgeMemoryGeneric. Done. > > https://codereview.chromium.org/1197753003/diff/40001/Source/wtf/PartitionAll... > Source/wtf/PartitionAlloc.cpp:1078: discardableBytes = bucket->slotSize - > usedBytes; > > Is it guaranteed that bucket->slotSize is a multiple of the system page size? Yes for cases where we store raw size. Currently, we store this for 64KB+ where the bucket spacing is 2 * system page for starters, and doubling from there. I added an ASSERT at the point of maximum coverage (partitionPageGetRawSizePtr) to be safe. > > https://codereview.chromium.org/1197753003/diff/40001/Source/wtf/PartitionAll... > Source/wtf/PartitionAlloc.cpp:1082: discardSystemPages(ptr, discardableBytes); > > For a single-slot allocation, we're committing the "trailing" system pages when > we allocated the page, and then discarding the "trailing" system pages here. > This looks a bit redundant. Can we avoid committing the "trailing" system pages > from the beginning? > > (We can do this in a follow-up.) That's a good question. We could de-commit / re-commit as appropriate at the time we're handing out an allocation from a pre-used single-slot allocation. My suspicion is that this wouldn't work for performance, though. I'll add a note to investigate to the PartitionAlloc document. > > https://codereview.chromium.org/1197753003/diff/40001/Source/wtf/PartitionAll... > Source/wtf/PartitionAlloc.cpp:1087: char slotUsage[(kPartitionPageSize * > kMaxPartitionPagesPerSlotSpan) / kSystemPageSize]; > > size_t maxSlotCount = (kPartitionPageSize * kMaxPartitionPagesPerSlotSpan) / > kSystemPageSize; > char slotUsage[maxSlotCount]; > > ... and then add ASSERT(slotIndex < maxSlotCount) when accessing the slotUsage > array. Done, but slightly differently: I added ASSERT(bucketNumSlots <= maxSlotCount); outside the loop and then the existing assert inside the loop should take care of business: ASSERT(slotIndex < bucketNumSlots); > > https://codereview.chromium.org/1197753003/diff/40001/Source/wtf/PartitionAll... > Source/wtf/PartitionAlloc.cpp:1091: PartitionFreelistEntry* fl = > page->freelistHead; > > fl => entry or freelist Done. > > https://codereview.chromium.org/1197753003/diff/40001/Source/wtf/PartitionAll... > Source/wtf/PartitionAlloc.cpp:1099: if (!fl && !partitionFreelistMask(fl)) > > Is it possible that this condition becomes true? I guess fl and > partitionFreelistMask(fl) don't become 0 at the same time. On little-endian, our mask function is bswap so they do both become 0 at the same time. On big-endian, our mask function is negate (~) so they do not become 0 at the same time and the optimization cannot apply. The important thing is whether the masked value is 0, which is what we are going to replace with a 0 after the discard. I'll remove the first condition and add a comment. I should have added a comment initially, sorry. > > To avoid the complexity, I might prefer just dropping the lastSlot optimization > :) Looking back at my notes, and if we trust the stats I collected, it was responsible for recovering about a third of the discardable memory so we should probably keep it. > > https://codereview.chromium.org/1197753003/diff/40001/Source/wtf/PartitionAll... > Source/wtf/PartitionAlloc.cpp:1136: (void) partitionPurgePage(page, true); > > Nit: (void) won't be needed. Not needed, but I like it to document that I've explicitly considered that I don't want to do anything with the return value.
haraken@chromium.org changed reviewers: + bashi@chromium.org
LGTM Once bashi@ implements a framework to collect memory data with memory-infra, it would be a good idea to measure the amount of memory saved by calling partitionPurgeMemory in the top 500 websites. It will give us good metrics we should optimize :) https://codereview.chromium.org/1197753003/diff/60001/Source/wtf/PartitionAll... File Source/wtf/PartitionAlloc.cpp (right): https://codereview.chromium.org/1197753003/diff/60001/Source/wtf/PartitionAll... Source/wtf/PartitionAlloc.cpp:1113: // we find that to be a win too. I think this is worth investigating, given the comment you wrote: > Looking back at my notes, and if we trust the stats I collected, it was responsible for recovering about a third of the discardable memory so we should > probably keep it. ^^^ This indicates that it is a key to remove unused PartitionFreelistEntries and thus discard system pages in which (only) the PartitionFreelistEntries reside.
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/1197753003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_blink_compile_dbg on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)
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/1197753003/#ps80001 (title: "Fix Windows compile error.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1197753003/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://src.chromium.org/viewvc/blink?view=rev&revision=197626 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
