Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
169523004:
Add partitionAllocGetSize() for determining actual size of allocation (Closed)
|
Created:
6 years, 10 months ago by Jens Widell Modified:
6 years, 10 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. |
DescriptionAdd partitionAllocGetSize() for determining actual size of allocation
Since PartitionAlloc allocates from buckets of quantized sizes, it will
sometimes allocate a significantly larger block than requested. Add the
utility function partitionAllocGetSize() for retrieving the actual size
of an allocation, useful if the client can make use of the extra size.
BUG=
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=167582
Patch Set 1 #
Total comments: 7
Patch Set 2 : address review comments #
Total comments: 3
Patch Set 3 : adjust test sizes, and reword/fix comments #
Messages
Total messages: 26 (0 generated)
Please consider this a draft; I'm uploading as a way of asking some questions. :-) 1) Do I need the spinLockLock(&root->lock) for reading page->bucket->slotSize? If not, I wouldn't need 'root' at all, I guess, and the function would work for non-generic allocations too. (Though I wouldn't think that would really be very interesting in practice.) 2) Sometimes bucket->slotSize is zero, right? Can I calculate the size from the page somehow then, or could we assume it to be within a system page or so of the requested size, and thus not too interesting? IOW, the API can be "if it returns zero, use the requested size". If there are more interesting tests you can think of, or particular size thresholds that would be interesting to test, please advise.
On 2014/02/17 12:22:24, Jens Lindström wrote: > Please consider this a draft; I'm uploading as a way of asking some questions. > :-) > > 1) Do I need the spinLockLock(&root->lock) for reading page->bucket->slotSize? > If not, I wouldn't need 'root' at all, I guess, and the function would work for > non-generic allocations too. (Though I wouldn't think that would really be very > interesting in practice.) Good question. I believe you do _not_ need the lock: - Once you have a PartitionPage*, that remains stable (there's one exception, see below). - Also, the design is such that a PartitionPage's bucket remains stable. - The exception is large allocations (currently the cutoff is a little under 1MB), where the PartitionPage and PartitionBucket get unmapped at free() time. - However, as a caller, it's your responsibility to have adequate locking and guarantees that the pointer you're asking about does not get asynchronously freed during the call. > > 2) Sometimes bucket->slotSize is zero, right? That used to be the case but should not be the case any more. If there's a case where it is zero, we have a bug. I recently fiddled around with the structures in order to make slotSize a uint32_t. (Used to be uint16_t). I'll add a few extra comments to the CL. > Can I calculate the size from the > page somehow then, or could we assume it to be within a system page or so of the > requested size, and thus not too interesting? IOW, the API can be "if it > returns zero, use the requested size". > > If there are more interesting tests you can think of, or particular size > thresholds that would be interesting to test, please advise.
Nice. I think we're pretty close design-wise now, not too much to do until we can land this. https://codereview.chromium.org/169523004/diff/1/Source/wtf/PartitionAlloc.h File Source/wtf/PartitionAlloc.h (right): https://codereview.chromium.org/169523004/diff/1/Source/wtf/PartitionAlloc.h#... Source/wtf/PartitionAlloc.h:571: ASSERT_NOT_REACHED(); I think this definitely will be reached. We want the client to call this API unconditionally, right? Hmm, I think the way to resolve this that doesn't add cost (branching, etc.) to the common case would be to add a new ALWAYS_INLINE API: partitionAllocSupportsGetSize(). It'd just return a hardwired true / false, depending on MEMORY_TOOL_REPLACES_ALLOCATOR, allowing the compiler to optimize fully. https://codereview.chromium.org/169523004/diff/1/Source/wtf/PartitionAlloc.h#... Source/wtf/PartitionAlloc.h:575: return 0; For simplicity, maybe try not supporting passing in NULL? I don't think the Vector client will need to do that, for example. https://codereview.chromium.org/169523004/diff/1/Source/wtf/PartitionAlloc.h#... Source/wtf/PartitionAlloc.h:579: spinLockLock(&root->lock); No need for locking, as covered separately. But definitely add a small comment to make it clear that the lack of locking is willful. https://codereview.chromium.org/169523004/diff/1/Source/wtf/PartitionAllocTes... File Source/wtf/PartitionAllocTest.cpp (right): https://codereview.chromium.org/169523004/diff/1/Source/wtf/PartitionAllocTes... Source/wtf/PartitionAllocTest.cpp:586: requestedSize = 512; I think you might need to adjust the size depending on whether it's debug or release. Otherwise you'll be testing two different underlying bucket sizes. You can use the existing "adjustment" constant as used elsewhere in the file. https://codereview.chromium.org/169523004/diff/1/Source/wtf/PartitionAllocTes... Source/wtf/PartitionAllocTest.cpp:586: requestedSize = 512; I have another idea. Can we make all the sizes odd? i.e. 511 instead of 512. We guarantee better-than-even alignment so we can tighten the conditions to EXPECT_LT(), gaining a better test. https://codereview.chromium.org/169523004/diff/1/Source/wtf/PartitionAllocTes... Source/wtf/PartitionAllocTest.cpp:603: requestedSize = 200 * 1024 + 250; A nicer size to test might be (256 * 1024) - kSystemPageSize. Should in theory round up to 256KB. For now, I'd also be OK hard-coding this round-up in to the test. I don't see the design ever changing such that a perfect power-of-two is not an exact allocation size. https://codereview.chromium.org/169523004/diff/1/Source/wtf/PartitionAllocTes... Source/wtf/PartitionAllocTest.cpp:613: // Allocate something very large. GetSize() should return zero. Does this actually happen? I see that you're testing with EXPECT_LE so maybe it does not return zero. It should not.
Thanks for great feedback! Changes: - Add *SupportsGetSize(), and simply assert in *GetSize() that it's supported. Callers will be required to check that it's supported before using. - Drop the locking and add comment saying essentially that we don't lock on purpose. - Adjust allocation sizes in the tests as suggested, and make the tests stricter (and more meaningful) by assuming more about the behavior of the underlying implementation.
LGTM subject to fixing three small nits before landing. https://codereview.chromium.org/169523004/diff/80001/Source/wtf/PartitionAllo... File Source/wtf/PartitionAllocTest.cpp (right): https://codereview.chromium.org/169523004/diff/80001/Source/wtf/PartitionAllo... Source/wtf/PartitionAllocTest.cpp:595: // Allocate that should be a perfect match for a bucket. Grammar: should be "allocation" or "allocate a size"? Also maybe add ", because it is an exact power of 2 size". https://codereview.chromium.org/169523004/diff/80001/Source/wtf/PartitionAllo... Source/wtf/PartitionAllocTest.cpp:603: // Allocate something slightly larger and uneven. GetSize() should return a The size is not uneven as far as I can see? Maybe just remove that part of the comment. Or replace it with "larger, but smaller than the bucket size" https://codereview.chromium.org/169523004/diff/80001/Source/wtf/PartitionAllo... Source/wtf/PartitionAllocTest.cpp:616: requestedSize = 512 * 1024 * 1024; Let's do (512 * 1024 * 1024) - 1, and then we can use EXPECT_LT as a tighter condition below :)
On 2014/02/19 07:15:51, Chris Evans wrote: > LGTM subject to fixing three small nits before landing. > > https://codereview.chromium.org/169523004/diff/80001/Source/wtf/PartitionAllo... > File Source/wtf/PartitionAllocTest.cpp (right): > > https://codereview.chromium.org/169523004/diff/80001/Source/wtf/PartitionAllo... > Source/wtf/PartitionAllocTest.cpp:595: // Allocate that should be a perfect > match for a bucket. > Grammar: should be "allocation" or "allocate a size"? > Also maybe add ", because it is an exact power of 2 size". > > https://codereview.chromium.org/169523004/diff/80001/Source/wtf/PartitionAllo... > Source/wtf/PartitionAllocTest.cpp:603: // Allocate something slightly larger and > uneven. GetSize() should return a > The size is not uneven as far as I can see? Maybe just remove that part of the > comment. Or replace it with "larger, but smaller than the bucket size" > > https://codereview.chromium.org/169523004/diff/80001/Source/wtf/PartitionAllo... > Source/wtf/PartitionAllocTest.cpp:616: requestedSize = 512 * 1024 * 1024; > Let's do (512 * 1024 * 1024) - 1, and then we can use EXPECT_LT as a tighter > condition below :) BTW I'd also be happy to review the subsequent patches in this series.
On 2014/02/19 07:15:51, Chris Evans wrote: > LGTM subject to fixing three small nits before landing. Tests adjusted as suggested. Thanks!
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/169523004/130001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_blink_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_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/169523004/130001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_blink_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_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/169523004/130001
The CQ bit was unchecked by jl@opera.com
I realized a simpler implementation of this functionality would be to simply calculate the actual size from the requested size, without involving a pointer to an actual allocation: size_t partitionAllocGetSize(size_t requestedSize); This could then be done before actually allocating anything. This would potentially be useful if someone is considering shrinking an allocation, to check if requesting a smaller memory block would actually produce a smaller memory block. It would also simplify the "we're not really using PartitionAlloc" case, since then we can just return the requested size, rather than needing to declare that we don't support the functionality. Chris, what do you think?
On 2014/02/20 08:58:09, Jens Lindström wrote: > I realized a simpler implementation of this functionality would be to simply > calculate the actual size from the requested size, without involving a pointer > to an actual allocation: > > size_t partitionAllocGetSize(size_t requestedSize); > > This could then be done before actually allocating anything. This would > potentially be useful if someone is considering shrinking an allocation, to > check if requesting a smaller memory block would actually produce a smaller > memory block. > > It would also simplify the "we're not really using PartitionAlloc" case, since > then we can just return the requested size, rather than needing to declare that > we don't support the functionality. > > Chris, what do you think? Interesting. To be safe you'd need a type-safe API, e.g. partitionAllocGenericGetSize(PartitionRootGeneric*, size_t) (This will avoid confusion with specific-sized partitions) And the implementation is partly based on partitionGenericSizeToBucket(root, size)->slotSize; But you'd need to cater for direct-mapped buckets and those just get rounded up to kSystemPageSize. In this branch, you'd need to look out for integer overflow. I'm kind of on the fence. The implementation for a ptr -> size mapping will likely be simpler, but if that leads to a more complex API for clients to use, that's uncool. I'd go with whatever you think will lead to a simpler / faster API for clients. I kind of lean towards the existing pointer-based API but feel free to go the other way.
On 2014/02/21 03:13:23, Chris Evans wrote: > Interesting. To be safe you'd need a type-safe API, e.g. > partitionAllocGenericGetSize(PartitionRootGeneric*, size_t) > (This will avoid confusion with specific-sized partitions) > > And the implementation is partly based on > partitionGenericSizeToBucket(root, size)->slotSize; > > But you'd need to cater for direct-mapped buckets and those just get rounded up > to > kSystemPageSize. In this branch, you'd need to look out for integer overflow. Good points. It wouldn't be quite as simple as I had imagined. > I'm kind of on the fence. The implementation for a ptr -> size mapping will > likely > be simpler, but if that leads to a more complex API for clients to use, that's > uncool. It's not really more complex for clients to use, but it limits what they can do. I was thinking primarily of Vector::shrinkCapacity(), which risks doing a pointless reallocation and copying operation while ending up with the same capacity as before. It currently checks whether the "quantized" reduced capacity is the same as the current capacity, but if we're dropping QuantizedAllocation in favor of using this API, Vector::shrinkCapacity() would have no mechanism to check in advance whether it can actually shrink the capacity as requested. Grepping the source code it seems that the function is used in quite a few places (called as Vector::shrinkToFit()) so we might be introducing a regression if we go ahead as planned. Of course, this problem with Vector::shrinkCapacity() might already exist to some degree; it could very well request a smaller "quantized" capacity, and still get a block from the same bucket, only not realizing it.
On 2014/02/21 06:01:59, Jens Lindström wrote: > On 2014/02/21 03:13:23, Chris Evans wrote: > > Interesting. To be safe you'd need a type-safe API, e.g. > > partitionAllocGenericGetSize(PartitionRootGeneric*, size_t) > > (This will avoid confusion with specific-sized partitions) > > > > And the implementation is partly based on > > partitionGenericSizeToBucket(root, size)->slotSize; > > > > But you'd need to cater for direct-mapped buckets and those just get rounded > up > > to > > kSystemPageSize. In this branch, you'd need to look out for integer overflow. > > Good points. It wouldn't be quite as simple as I had imagined. > > > I'm kind of on the fence. The implementation for a ptr -> size mapping will > > likely > > be simpler, but if that leads to a more complex API for clients to use, that's > > uncool. > > It's not really more complex for clients to use, but it limits what they can do. > > I was thinking primarily of Vector::shrinkCapacity(), which risks doing a > pointless reallocation and copying operation while ending up with the same > capacity as before. It currently checks whether the "quantized" reduced > capacity is the same as the current capacity, but if we're dropping > QuantizedAllocation in favor of using this API, Vector::shrinkCapacity() would > have no mechanism to check in advance whether it can actually shrink the > capacity as requested. Ah, a compelling point! I think shrinkCapacity() should really be wiring through to partitionReallocGeneric() and simply checking if the pointer value changed. (This would also encourage us to add not-yet-done optimizations in PartitionAlloc such as in-place resizing of any direct mapped allocation.) Shorter-term, it sounds like adding partitionAllocGetSize(size_t) for this case is fine. Longer-term, maybe we should have both variants of the API so that clients have all the options. Since you've done the work for the void* variant already, I have no problem with you landing it even if no-one uses it right away. We could even proceed with the size_t variant in a different CL. > > Grepping the source code it seems that the function is used in quite a few > places (called as Vector::shrinkToFit()) so we might be introducing a regression > if we go ahead as planned. > > Of course, this problem with Vector::shrinkCapacity() might already exist to > some degree; it could very well request a smaller "quantized" capacity, and > still get a block from the same bucket, only not realizing it.
On 2014/02/21 08:32:48, Chris Evans wrote: > On 2014/02/21 06:01:59, Jens Lindström wrote: > > On 2014/02/21 03:13:23, Chris Evans wrote: > > > Interesting. To be safe you'd need a type-safe API, e.g. > > > partitionAllocGenericGetSize(PartitionRootGeneric*, size_t) > > > (This will avoid confusion with specific-sized partitions) > > > > > > And the implementation is partly based on > > > partitionGenericSizeToBucket(root, size)->slotSize; > > > > > > But you'd need to cater for direct-mapped buckets and those just get rounded > > up > > > to > > > kSystemPageSize. In this branch, you'd need to look out for integer > overflow. > > > > Good points. It wouldn't be quite as simple as I had imagined. > > > > > I'm kind of on the fence. The implementation for a ptr -> size mapping will > > > likely > > > be simpler, but if that leads to a more complex API for clients to use, > that's > > > uncool. > > > > It's not really more complex for clients to use, but it limits what they can > do. > > > > I was thinking primarily of Vector::shrinkCapacity(), which risks doing a > > pointless reallocation and copying operation while ending up with the same > > capacity as before. It currently checks whether the "quantized" reduced > > capacity is the same as the current capacity, but if we're dropping > > QuantizedAllocation in favor of using this API, Vector::shrinkCapacity() would > > have no mechanism to check in advance whether it can actually shrink the > > capacity as requested. > > Ah, a compelling point! > > I think shrinkCapacity() should really be wiring through to > partitionReallocGeneric() and simply checking if the pointer value changed. Yes, that sounds like a good idea. I might even look into that next. :-) > Shorter-term, it sounds like adding partitionAllocGetSize(size_t) for this case > is fine. Longer-term, maybe we should have both variants of the API so that > clients have all the options. Since you've done the work for the void* variant > already, I have no problem with you landing it even if no-one uses it right > away. We could even proceed with the size_t variant in a different CL. I'll go ahead and land this as-is, and upload the follow-ups that make use of it. Thanks!
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/169523004/130001
Message was sent while issue was closed.
Change committed as 167582 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
