PA: Replace FreeWithSize() with Free() with FreeFlags::kWithSizeHint. [chromium/src : main]
Takashi Sakamoto (Gerrit)
Takashi Sakamoto added 1 comment![]( "Open in Gerrit")
Related details
- Keishi Hattori
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takashi Sakamoto (Gerrit)
Takashi Sakamoto added 1 comment![]( "Open in Gerrit")
uint32_t type_id = 0;I think this will be replaced with alloc_token (or add new member for alloc_token).
Related details
- Keishi Hattori
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen Nusko (Gerrit)
Stephen Nusko added 4 comments![]( "Open in Gerrit")
new_hint.size = adjusted_size;This is what I mean by we could adjust it if it was taken by value?
FreeHint new_hint = hint;If we took the hint by value, rather than by const ref we could just update the one field that changed?
PA_NOINLINE void Free(void* object, const FreeHint& hint) {Is it better to take these as value rather than const ref?
It is quite a simple struct and then we could modify it for example when we adjust the size?
partition_alloc::FreeHint hint;
hint.size = size;I think we can improve the readability a bit with either helper functions or initialier.
some helper functions would look like this?
`partition_alloc::FreeHint::Size(size)` -> returns a FreeHint with size set
`partition_alloc::FreeHint::Alignment(alignment)` -> returns a FreeHint alignment set.
`partition_alloc::FreeHint::SizeAndAlignment(size, alignment)` -> returns with both set.
Since we do this frequently might make it read a bit better everywhere? And since there isn't that many combos it shouldn't be that bad.
Bonus we could even have it cache the FreeFlags that it has.
I.E. FreeHint::Size would set a field `const kFlags = FreeFlags::kWithSizeHint`
And then we would just do
`FreeInlineInUnknownRoot<base_free_flags | hint.flags>(object, hint);`
The other suggestion would be to use initializers
```c++
partition_alloc::FreeHint hint{
.size = size,
};
```
Related details
- Keishi Hattori
- Takashi Sakamoto
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keishi Hattori (Gerrit)
Keishi Hattori added 3 comments![]( "Open in Gerrit")
PA_PREFETCH(slot_span);Don't we need to prefetch if settings_.enable_free_with_size is false?
PA_ALWAYS_INLINE void PartitionRoot::FreeInline(void* object,If kWithAlignmentHint is set don't we always require kWithSizeHint to be set? If so maybe we can have a static assert for that.
Related details
- Takashi Sakamoto
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takashi Sakamoto (Gerrit)
Takashi Sakamoto added 9 comments![]( "Open in Gerrit")
kMaxValue = kIntendedLeak,I'm not sure whether `internal`-s should be alreays the last ones in the `FreeFlags` enum or not.
Did you forget to pass in |hint|?
Done
PA_PREFETCH(slot_span);Don't we need to prefetch if settings_.enable_free_with_size is false?
The original `FreeInline<flags>(void* object)` doesn't check `settings_.enable_free_with_size`, c.f.
```
template <FreeFlags flags>
PA_ALWAYS_INLINE void PartitionRoot::FreeInline(void* object) {
// The correct PartitionRoot might not be deducible if the |object| originates
// from an override hook.
bool early_return = FreeProlog<flags>(object, this);
if (early_return) {
return;
}
// FreeProlog ensures the object is not nullptr.
PA_DCHECK(object);
// Almost all calls to FreeNoNooks() will end up writing to |*object|.
PA_PREFETCH_FOR_WRITE(object);
auto [slot_start, slot_span] = GetSlotStartAndSlotSpanFromAddress(object);
// We are going to read from |*slot_span| in all branches, but haven't
// done it yet.
PA_PREFETCH(slot_span);
FreeNoHooksImmediate<flags>(slot_start, slot_span);
}
```
The `!ContainsFlags(flags, FreeFlags::kWithSizeHint)` means the original `FreeInline()` is invoked. Neither `FreeWithSizeInline()` nor `FreeWithSizeAlignmentInline()` is invoked.
So since the original `FreeInline()` does not check `settings_.enable_free_with_size`, I think we can skip checking `settings_.enable_free_with_size` here.
(If we call `FreeWithSizeInline()` or `FreeWithSizeAlignmentInline()`,
```
template <FreeFlags flags>
PA_ALWAYS_INLINE void PartitionRoot::FreeWithSizeInline(void* object,
size_t size) {
if (!settings_.enable_free_with_size) {
FreeInline<flags>(object);
return;
}
... snip
```
and
```
template <FreeFlags flags>
PA_ALWAYS_INLINE void PartitionRoot::FreeWithSizeAndAlignmentInline(
void* object,
size_t size,
size_t alignment) {
if (!settings_.enable_free_with_size) {
FreeInline<flags>(object); // FreeInline()
return;
}
... snip
```
we need to check `settings_.enable_free_with_size`.)
This is what I mean by we could adjust it if it was taken by value?
Done
If we took the hint by value, rather than by const ref we could just update the one field that changed?
Done.
I will check the binary (size and generated asm) when `FreeHintType` is much larger.
PA_ALWAYS_INLINE void PartitionRoot::FreeInline(void* object,If kWithAlignmentHint is set don't we always require kWithSizeHint to be set? If so maybe we can have a static assert for that.
Currently PartitionAlloc `AlignedFree` doesn't take `alignment`.
So thinking about normal allocator, `AlignedFree` will take `alignment` argument and `FreeWithSize` will take `size` argument. `AlignedFreeWithSize` will take both.
(The arguments don't depend on each other.)
But because of PartitionAlloc impl, only `FreeWithSizeAndAlignment()` needs `alignment`.... I'm not sure whether we can say `alignment requires size` or not.
To avoid mistakes, now `template struct FreeHint` captures such kind of `missing size` or `missing alignment`, because `FreeHint<FreeFlags::kWithAlignmentHint>::Type` has no `size` member (i.e. different type from `FreeHint<FreeFlags::kWithSizeHint>::Type`).
PA_NOINLINE void Free(void* object, const FreeHint& hint) {Is it better to take these as value rather than const ref?
It is quite a simple struct and then we could modify it for example when we adjust the size?
I made FreeHintType template struct and changed Free() to take values.
Looking at binaries, using values instead of const ref seems to increase the size of binaries. (clang seems to optimize the binary to avoid lots of copies, but sometimes the code seems to need copies). But I found that always initializing unused struct members costs a lot...
i.e.
```
struct FreeHint {
const char* type_name = nullptr;
uint32_t type_id = 0;
size_t size = 0;
size_t alignment = 0;
};
FreeHint hint;
hint.size = size;
00000000004ad180 <partition_alloc::internal::(anonymous namespace)::PartitionAllocatorWithThreadCache::FreeWithSize(void*, unsigned long)>:
4ad180: 55 push rbp
4ad181: 48 89 e5 mov rbp, rsp
4ad184: 48 83 ec 20 sub rsp, 0x20
4ad188: 48 89 f7 mov rdi, rsi
4ad18b: 48 b8 aa aa aa aa aa aa aa aa movabs rax, -0x5555555555555556
4ad195: 48 89 45 e8 mov qword ptr [rbp - 0x18], rax
4ad199: 48 c7 45 e0 00 00 00 00 mov qword ptr [rbp - 0x20], 0x0
4ad1a1: c7 45 e8 00 00 00 00 mov dword ptr [rbp - 0x18], 0x0
4ad1a8: 48 c7 45 f8 00 00 00 00 mov qword ptr [rbp - 0x8], 0x0
4ad1b0: 48 89 55 f0 mov qword ptr [rbp - 0x10], rdx
4ad1b4: 48 8d 75 e0 lea rsi, [rbp - 0x20]
4ad1b8: e8 83 49 39 00 call 0x841b40 <void partition_alloc::PartitionRoot::FreeInUnknownRoot<(partition_alloc::internal::FreeFlags)18>(void*, partition_alloc::FreeHint const&)>
4ad1bd: 48 83 c4 20 add rsp, 0x20
4ad1c1: 5d pop rbp
4ad1c2: c3 ret
```
If we don't initialize the struct members,
```
struct FreeHint {
// Members are intentionally left uninitialized for performance
// reason.
// If using zero-initialization,
// FreeHinit hint
const char* type_name;
uint32_t type_id;
size_t size;
size_t alignment;
};
FreeHint hint;
hint.size = size;
00000000004ad180 <partition_alloc::internal::(anonymous namespace)::PartitionAllocatorWithThreadCache::FreeWithSize(void*, unsigned long)>:
4ad180: 55 push rbp
4ad181: 48 89 e5 mov rbp, rsp
4ad184: 48 83 ec 20 sub rsp, 0x20
4ad188: 48 89 f7 mov rdi, rsi
4ad18b: 0f 28 05 be 4d d2 ff movaps xmm0, xmmword ptr [rip - 0x2db242] # 0x1d1f50 <gPublicTypes+0x7c0>
4ad192: 0f 29 45 f0 movaps xmmword ptr [rbp - 0x10], xmm0
4ad196: 0f 29 45 e0 movaps xmmword ptr [rbp - 0x20], xmm0
4ad19a: 48 89 55 f0 mov qword ptr [rbp - 0x10], rdx
4ad19e: 48 8d 75 e0 lea rsi, [rbp - 0x20]
4ad1a2: e8 79 49 39 00 call 0x841b20 <void partition_alloc::PartitionRoot::FreeInUnknownRoot<(partition_alloc::internal::FreeFlags)18>(void*, partition_alloc::FreeHint const&)>
4ad1a7: 48 83 c4 20 add rsp, 0x20
4ad1ab: 5d pop rbp
4ad1ac: c3 ret
```
So if the struct looks like:
```
struct FreeHint {
size_t size = 0;
};
00000000004ad540 <partition_alloc::internal::(anonymous namespace)::PartitionAllocatorWithThreadCache::FreeWithSize(void*, unsigned long)>:
4ad540: 55 push rbp
4ad541: 48 89 e5 mov rbp, rsp
4ad544: 48 89 f7 mov rdi, rsi
4ad547: 48 89 d6 mov rsi, rdx
4ad54a: 5d pop rbp
4ad54b: e9 10 61 39 00 jmp 0x843660 <void partition_alloc::PartitionRoot::FreeInUnknownRoot<(partition_alloc::internal::FreeFlags)18>(void*, partition_alloc::FreeHint<partition_alloc::internal::FreeFlags partition_alloc::internal::FreeHintFlags<partition_alloc::internal::FreeFlags>(partition_alloc::internal::FreeFlags)((partition_alloc::internal::FreeFlags)18)>::Type)>
```
So now `Free<kWithSizeHint>(void*, FreeHint)` is almost the same as `FreeWithSize(void*, size_t)`.
I think we can improve the readability a bit with either helper functions or initialier.
some helper functions would look like this?
`partition_alloc::FreeHint::Size(size)` -> returns a FreeHint with size set
`partition_alloc::FreeHint::Alignment(alignment)` -> returns a FreeHint alignment set.
`partition_alloc::FreeHint::SizeAndAlignment(size, alignment)` -> returns with both set.Since we do this frequently might make it read a bit better everywhere? And since there isn't that many combos it shouldn't be that bad.
Bonus we could even have it cache the FreeFlags that it has.
I.E. FreeHint::Size would set a field `const kFlags = FreeFlags::kWithSizeHint`
And then we would just do
`FreeInlineInUnknownRoot<base_free_flags | hint.flags>(object, hint);`The other suggestion would be to use initializers
```c++
partition_alloc::FreeHint hint{
.size = size,
};
```
I see. Done.
Now my concern is clang plugin: `complex class or struct`.
Related details
- Keishi Hattori
- Stephen Nusko
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keishi Hattori (Gerrit)
Keishi Hattori voted and added 1 comment![]( "Open in Gerrit")
Votes added by Keishi Hattori
| Code-Review | +1 |
1 comment
Related details
- Stephen Nusko
- Takashi Sakamoto
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takashi Sakamoto (Gerrit)
Takashi Sakamoto added 6 comments![]( "Open in Gerrit")
Thank you for the review.
kMaxValue = kIntendedLeak,I'm not sure whether `internal`-s should be alreays the last ones in the `FreeFlags` enum or not.
Acknowledged
PA_PREFETCH(slot_span);Acknowledged
PA_ALWAYS_INLINE void PartitionRoot::FreeInline(void* object,Takashi SakamotoIf kWithAlignmentHint is set don't we always require kWithSizeHint to be set? If so maybe we can have a static assert for that.
Currently PartitionAlloc `AlignedFree` doesn't take `alignment`.
So thinking about normal allocator, `AlignedFree` will take `alignment` argument and `FreeWithSize` will take `size` argument. `AlignedFreeWithSize` will take both.
(The arguments don't depend on each other.)But because of PartitionAlloc impl, only `FreeWithSizeAndAlignment()` needs `alignment`.... I'm not sure whether we can say `alignment requires size` or not.
To avoid mistakes, now `template struct FreeHint` captures such kind of `missing size` or `missing alignment`, because `FreeHint<FreeFlags::kWithAlignmentHint>::Type` has no `size` member (i.e. different type from `FreeHint<FreeFlags::kWithSizeHint>::Type`).
Acknowledged
PA_NOINLINE void Free(void* object, const FreeHint& hint) {Acknowledged
I think this will be replaced with alloc_token (or add new member for alloc_token).
Acknowledged
Related details
- Stephen Nusko
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takashi Sakamoto (Gerrit)
Takashi Sakamoto voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
PA: Replace FreeWithSize() with Free() with FreeFlags::kWithSizeHint.
Add template struct FreeHint to provide additional information at
Free(), e.g. size. The template parameter is the same as PartitionRoot
Free()'s, i.e. `template <FreeFlags flags> Free(void*,
FreeHint<flags>::Type)`. If FreeFlags contains kWithSizeHint (constexpr
ContainsFlags), FreeHint<kWithSizeHint|...>::Type will have size member
and PartitionRoot::Free() can use its value (otherwise, no size member).
This template struct helps us to avoid initializing and copying unused
struct members.
- M base/allocator/partition_allocator/src/partition_alloc/BUILD.gn
- A base/allocator/partition_allocator/src/partition_alloc/free_hint.h
- M base/allocator/partition_allocator/src/partition_alloc/internal/partition_root_exports.h
- M base/allocator/partition_allocator/src/partition_alloc/internal/partition_root_internal.h
- M base/allocator/partition_allocator/src/partition_alloc/partition_alloc_constants.h
- M base/allocator/partition_allocator/src/partition_alloc/partition_alloc_perftest.cc
- M base/allocator/partition_allocator/src/partition_alloc/partition_alloc_unittest.cc
- M base/allocator/partition_allocator/src/partition_alloc/partition_root.cc
- M base/allocator/partition_allocator/src/partition_alloc/partition_root.h
- M base/allocator/partition_allocator/src/partition_alloc/shim/allocator_shim_default_dispatch_to_partition_alloc.cc
- M third_party/blink/renderer/platform/wtf/allocator/partitions.cc
Code-Review: +1 by Keishi Hattori
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |