Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
183113003:
PartitionAlloc: support in-place resize of directly mapped allocation (Closed)
|
Created:
6 years, 9 months ago by Jens Widell Modified:
6 years, 9 months ago Reviewers:
Chris Evans CC:
blink-reviews, loislo+blink_chromium.org, yurys+blink_chromium.org, abarth-chromium, adamk+blink_chromium.org, Inactive, Mikhail Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionPartitionAlloc: support in-place resize of directly mapped allocation
In partitionReallocGeneric(), if both the current size and the new size
are in the direct mapped range, attempt to resize the allocation in place.
When downsizing, this is done by decommitting pages and making them
inaccessible. When upsizing a previously downsized allocation to a size
not larger than its original size, we make the pages accessible again.
R=cevans@chromium.org
BUG=
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=168633
Patch Set 1 #
Total comments: 2
Patch Set 2 : fix various issues #
Total comments: 12
Patch Set 3 : assorted simple fixes #Patch Set 4 : redesign a bit as suggested #
Total comments: 7
Patch Set 5 : addressing some more comments #
Total comments: 3
Patch Set 6 : nits addressed #
Messages
Total messages: 24 (0 generated)
A working implementation (I think) but a bit of an RFC on the design. The abuse of freePagesHead is a bit ugly. Needed somewhere to record the original size of the direct mapping. Since freePagesHead is only used in PartitionAlloc.cpp and only in slow paths, it seemed the best option for abuse. If someone tries to interpret it as a free page, they shall die quickly since it always points to an inaccessible page (when abused.) Anyhow, better suggestions are welcome, of course.
https://codereview.chromium.org/183113003/diff/1/Source/wtf/PartitionAlloc.cpp File Source/wtf/PartitionAlloc.cpp (right): https://codereview.chromium.org/183113003/diff/1/Source/wtf/PartitionAlloc.cp... Source/wtf/PartitionAlloc.cpp:589: size_t unmapSize = page->bucket->slotSize; This would need adjusting to use page->bucket->freePagesHead instead; page->bucket->slotSize is the size of current allocation and doesn't necessarily represent what we actually mapped and should unmap.
Finding flaws. :-) https://codereview.chromium.org/183113003/diff/1/Source/wtf/PartitionAlloc.cpp File Source/wtf/PartitionAlloc.cpp (right): https://codereview.chromium.org/183113003/diff/1/Source/wtf/PartitionAlloc.cp... Source/wtf/PartitionAlloc.cpp:784: setSystemPagesInaccessible(realPtr + newSize, kSystemPageSize); Ought to make all decommitted pages inaccessible (as the comment states,) not just one of them.
Sorry for the slow response time. Some initial comments! https://codereview.chromium.org/183113003/diff/20001/Source/wtf/PartitionAllo... File Source/wtf/PartitionAlloc.cpp (right): https://codereview.chromium.org/183113003/diff/20001/Source/wtf/PartitionAllo... Source/wtf/PartitionAlloc.cpp:527: static ALWAYS_INLINE size_t partitionDirectMapAvailableSize(PartitionPage* page) This function seems a bit complicated, or at least large. Ideally we can do without it and fetch the size directly as a struct member, possibly as I describe below in a comment inside partitionDirectMap. https://codereview.chromium.org/183113003/diff/20001/Source/wtf/PartitionAllo... Source/wtf/PartitionAlloc.cpp:602: bucket->freePagesHead = reinterpret_cast<PartitionPage*>(ptr + mapSize); I'm not a huge fan of this. For each direct mapping, we have a whole system page in which to put metadata about the mapping, and it's not very full yet :-) It looks like we need a new piece of metadata to describe the available contiguous mapping size, which is different from the bucket->slotsize. So how about some "struct PartitionDirectMapExtent"? Initially it can just store the mapping size (not include guard pages etc.) and we'd set it to the same as bucket->slotSize to start with. We can store it at reinterpret_cast<PartitionBucket*>(reinterpret_cast<char*>(page) + kPageMetadataSize * 2) When pulling this trick, we might also want to add a new COMPILER_ASSERT that the size of PartitionBucket is also <= kPageMetadataSize https://codereview.chromium.org/183113003/diff/20001/Source/wtf/PartitionAllo... Source/wtf/PartitionAlloc.cpp:795: ASSERT(partitionDirectMapSize(newSize) == newSize); I think we also need newSize to be a multiple of kSystemPageSize here; might as well ASSERT it? https://codereview.chromium.org/183113003/diff/20001/Source/wtf/PartitionAllo... Source/wtf/PartitionAlloc.cpp:824: // We can't perform the realloc in-place. I think we can fill in this branch in the future (probably not this CL, it's complicated). Unfortunately, I think this is the branch which is most important, particularly if we ever wired Vector and HashTable to realloc(). It's complicated mainly because of the Windows API for mapping pages :-( https://codereview.chromium.org/183113003/diff/20001/Source/wtf/PartitionAllo... Source/wtf/PartitionAlloc.cpp:847: } Can we just put a no nonsense RELEASE_ASSERT here? e.g. RELEASE_ASSERT(newSize <= INT_MAX - kSystemPageSize) Should prevent all future possible accidents in this area :) https://codereview.chromium.org/183113003/diff/20001/Source/wtf/PartitionAllo... Source/wtf/PartitionAlloc.cpp:864: if (partitionSizeIsDirectMapped(actualOldSize) && partitionSizeIsDirectMapped(actualNewSize)) { I actually think we might want to relax the second condition. For example, using current tuning values, let's say we: - Alloc 1MB (this will be direct mapped) - realloc to 500KB (will get pushed into a super page allocation) I don't think that's what we'd want in this case as it results in a copy. A sensible threshold is probably something like 128KB. (The worry of making the threshold too low is that the 4KB metadata page becomes too large as a percentage of waste.) https://codereview.chromium.org/183113003/diff/20001/Source/wtf/PartitionAlloc.h File Source/wtf/PartitionAlloc.h (right): https://codereview.chromium.org/183113003/diff/20001/Source/wtf/PartitionAllo... Source/wtf/PartitionAlloc.h:574: ALWAYS_INLINE bool partitionSizeIsDirectMapped(size_t size) I think we can make this private by moving to the .cpp (see comment in test file). https://codereview.chromium.org/183113003/diff/20001/Source/wtf/PartitionAllo... Source/wtf/PartitionAlloc.h:577: return size > kGenericMaxBucketed && size <= kGenericMaxDirectMapped; I think the second condition is a bit subtle here. I think I prefer a direct RELEASE_ASSERT() check against newSize in realloc itself? https://codereview.chromium.org/183113003/diff/20001/Source/wtf/PartitionAllo... File Source/wtf/PartitionAllocTest.cpp (right): https://codereview.chromium.org/183113003/diff/20001/Source/wtf/PartitionAllo... Source/wtf/PartitionAllocTest.cpp:697: EXPECT_TRUE(WTF::partitionSizeIsDirectMapped(size)); We can avoid making this a public API by getting the PartitionPage and PartitionBucket from the returned pointer and using the existing partitionBucketIsDirectMapped.
https://codereview.chromium.org/183113003/diff/20001/Source/wtf/PartitionAllo... File Source/wtf/PartitionAlloc.cpp (right): https://codereview.chromium.org/183113003/diff/20001/Source/wtf/PartitionAllo... Source/wtf/PartitionAlloc.cpp:602: bucket->freePagesHead = reinterpret_cast<PartitionPage*>(ptr + mapSize); On 2014/03/05 07:57:04, Chris Evans wrote: > I'm not a huge fan of this. I certainly wasn't either. :-) > So how about some "struct PartitionDirectMapExtent"? Initially it can just store > the mapping size (not include guard pages etc.) and we'd set it to the same as > bucket->slotSize to start with. > > We can store it at > reinterpret_cast<PartitionBucket*>(reinterpret_cast<char*>(page) + > kPageMetadataSize * 2) Yeah, I'll do that instead. https://codereview.chromium.org/183113003/diff/20001/Source/wtf/PartitionAllo... Source/wtf/PartitionAlloc.cpp:795: ASSERT(partitionDirectMapSize(newSize) == newSize); On 2014/03/05 07:57:04, Chris Evans wrote: > I think we also need newSize to be a multiple of kSystemPageSize here; might as > well ASSERT it? The value returned by partitionDirectMapSize(size) is effectively simply 'size' rounded up to a system page size. A more explicit assert could serve as better code documentation, though. https://codereview.chromium.org/183113003/diff/20001/Source/wtf/PartitionAllo... Source/wtf/PartitionAlloc.cpp:864: if (partitionSizeIsDirectMapped(actualOldSize) && partitionSizeIsDirectMapped(actualNewSize)) { On 2014/03/05 07:57:04, Chris Evans wrote: > I actually think we might want to relax the second condition. For example, using > current tuning values, let's say we: > - Alloc 1MB (this will be direct mapped) > - realloc to 500KB (will get pushed into a super page allocation) The reason I didn't do that was to avoid having an allocation that is too small to be a direct map but still is one, which could confuse some condition somewhere else. But we should of course use partitionBucketIsDirectMapped() and not the size of the allocation to determine that anyway, so it's probably not a problem in practice. I'll change it.
On 2014/03/05 08:11:42, Jens Lindström wrote: > https://codereview.chromium.org/183113003/diff/20001/Source/wtf/PartitionAllo... > File Source/wtf/PartitionAlloc.cpp (right): > > https://codereview.chromium.org/183113003/diff/20001/Source/wtf/PartitionAllo... > Source/wtf/PartitionAlloc.cpp:602: bucket->freePagesHead = > reinterpret_cast<PartitionPage*>(ptr + mapSize); > On 2014/03/05 07:57:04, Chris Evans wrote: > > I'm not a huge fan of this. > > I certainly wasn't either. :-) > > > So how about some "struct PartitionDirectMapExtent"? Initially it can just > store > > the mapping size (not include guard pages etc.) and we'd set it to the same as > > bucket->slotSize to start with. > > > > We can store it at > > reinterpret_cast<PartitionBucket*>(reinterpret_cast<char*>(page) + > > kPageMetadataSize * 2) > > Yeah, I'll do that instead. > > https://codereview.chromium.org/183113003/diff/20001/Source/wtf/PartitionAllo... > Source/wtf/PartitionAlloc.cpp:795: ASSERT(partitionDirectMapSize(newSize) == > newSize); > On 2014/03/05 07:57:04, Chris Evans wrote: > > I think we also need newSize to be a multiple of kSystemPageSize here; might > as > > well ASSERT it? > > The value returned by partitionDirectMapSize(size) is effectively simply 'size' > rounded up to a system page size. A more explicit assert could serve as better > code documentation, though. > > https://codereview.chromium.org/183113003/diff/20001/Source/wtf/PartitionAllo... > Source/wtf/PartitionAlloc.cpp:864: if > (partitionSizeIsDirectMapped(actualOldSize) && > partitionSizeIsDirectMapped(actualNewSize)) { > On 2014/03/05 07:57:04, Chris Evans wrote: > > I actually think we might want to relax the second condition. For example, > using > > current tuning values, let's say we: > > - Alloc 1MB (this will be direct mapped) > > - realloc to 500KB (will get pushed into a super page allocation) > > The reason I didn't do that was to avoid having an allocation that is too small > to be a direct map but still is one, which could confuse some condition Ah yes. I think this is telling us that we shouldn't have a partitionSizeIsDirectMapped() at all. In the realloc() code, we should just grab the PartitionBucket for the old ptr and use partitionBucketIsDirectMapped() for our decision. > somewhere else. But we should of course use partitionBucketIsDirectMapped() and > not the size of the allocation to determine that anyway, so it's probably not a > problem in practice. > > I'll change it.
Patch updated to address all the comments so far (I think.) Introduced a "struct PartitionDirectMapExtent" as suggested, and stopped abusing freePagesHead. Dropped partitionSizeIsDirectMapped(). Also support downsizing all the way down to 10*kPartitionPageSize (<= 10% waste).
Getting close! https://codereview.chromium.org/183113003/diff/50001/Source/wtf/PartitionAllo... File Source/wtf/PartitionAlloc.cpp (right): https://codereview.chromium.org/183113003/diff/50001/Source/wtf/PartitionAllo... Source/wtf/PartitionAlloc.cpp:791: PartitionPage* page = partitionPointerToPage(ptr); Pass in page instead of ptr to this function to avoid this small piece of work. https://codereview.chromium.org/183113003/diff/50001/Source/wtf/PartitionAllo... Source/wtf/PartitionAlloc.cpp:797: if (newSize == bucket->slotSize) This check looks duplicated with a check in the parent function. Can we remove this check by re-ordering the parent function slightly? We could even ASSERT that this is then _not_ equal. https://codereview.chromium.org/183113003/diff/50001/Source/wtf/PartitionAllo... Source/wtf/PartitionAlloc.cpp:843: RELEASE_ASSERT(newSize <= INT_MAX - kSystemPageSize); The RHS is just kGenericMaxDirectMapped, I think. https://codereview.chromium.org/183113003/diff/50001/Source/wtf/PartitionAllo... Source/wtf/PartitionAlloc.cpp:850: if (partitionBucketIsDirectMapped(page->bucket)) { Might as well wrap that condition in an UNLIKELY ? https://codereview.chromium.org/183113003/diff/50001/Source/wtf/PartitionAllo... Source/wtf/PartitionAlloc.cpp:854: if (partitionReallocDirectMappedInPlace(root, realPtr, newSize)) Pass in the page and not the pointer to do a little less work. https://codereview.chromium.org/183113003/diff/50001/Source/wtf/PartitionAlloc.h File Source/wtf/PartitionAlloc.h (right): https://codereview.chromium.org/183113003/diff/50001/Source/wtf/PartitionAllo... Source/wtf/PartitionAlloc.h:186: static const size_t kGenericMinDirectMapped = 10 * kPartitionPageSize; // Limit when downsizing a direct mapping using realloc(); allows 10 % allocation overhead. At the risk of giving away my OCD, can we make that 16 * blah? A nice power of two? :P Also, can we work the term "downsize" into the name to make it more apparent when we use this? e.g. kGenericMinDirectMappedDownsize or some such?
https://codereview.chromium.org/183113003/diff/50001/Source/wtf/PartitionAllo... File Source/wtf/PartitionAlloc.cpp (right): https://codereview.chromium.org/183113003/diff/50001/Source/wtf/PartitionAllo... Source/wtf/PartitionAlloc.cpp:797: if (newSize == bucket->slotSize) On 2014/03/06 05:55:13, Chris Evans wrote: > This check looks duplicated with a check in the parent function. Can we remove > this check by re-ordering the parent function slightly? We could even ASSERT > that this is then _not_ equal. If you mean the actualOldSize/actualNewSize comparison in the parent function, I actually reordered and introduced this check in the latest version of the patch. The values in the comparison in the parent function are rounded to bucket sizes (when below gGenericMaxBucketed) which doesn't really make sense if we're downsizing a direct mapping.
On 2014/03/06 05:55:13, Chris Evans wrote: > Getting close! Thanks for great feedback! :-) Patch updated as suggested (mostly.)
LGTM 2 nits; do with them as you please. No need to re-review. https://codereview.chromium.org/183113003/diff/60001/Source/wtf/PartitionAllo... File Source/wtf/PartitionAlloc.cpp (right): https://codereview.chromium.org/183113003/diff/60001/Source/wtf/PartitionAllo... Source/wtf/PartitionAlloc.cpp:50: COMPILE_ASSERT(sizeof(WTF::PartitionBucket) <= WTF::kPageMetadataSize, PartitionBucket_not_too_big); Nit: maybe put this directly under PartitionPa ge_not_too_big https://codereview.chromium.org/183113003/diff/60001/Source/wtf/PartitionAllo... Source/wtf/PartitionAlloc.cpp:791: ASSERT(partitionBucketIsDirectMapped(page->bucket)); Nit: maybe put the ASSERT at the top of the function, so it always executes regardless of the above branch. I think it's an invariant.
One more nit. https://codereview.chromium.org/183113003/diff/60001/Source/wtf/PartitionAllo... File Source/wtf/PartitionAlloc.cpp (right): https://codereview.chromium.org/183113003/diff/60001/Source/wtf/PartitionAllo... Source/wtf/PartitionAlloc.cpp:847: if (partitionBucketIsDirectMapped(page->bucket)) { Nit: could add UNLIKELY around this condition, as suggested in previous iteration of feedback.
On 2014/03/06 06:31:22, Chris Evans wrote: > One more nit. > > https://codereview.chromium.org/183113003/diff/60001/Source/wtf/PartitionAllo... > File Source/wtf/PartitionAlloc.cpp (right): > > https://codereview.chromium.org/183113003/diff/60001/Source/wtf/PartitionAllo... > Source/wtf/PartitionAlloc.cpp:847: if > (partitionBucketIsDirectMapped(page->bucket)) { > Nit: could add UNLIKELY around this condition, as suggested in previous > iteration of feedback. Sorry, didn't mean to ignore that suggestion. Opera's code review system doesn't let you forget things as easily; I guess I've trained myself to depend on that... :-)
The CQ bit was checked by jl@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jl@opera.com/183113003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_blink
The CQ bit was checked by jl@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jl@opera.com/183113003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_blink
The CQ bit was checked by jl@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jl@opera.com/183113003/80001
Message was sent while issue was closed.
Change committed as 168633 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
