Reland "PA: Support multiple ThreadCache instances per thread." [chromium/src : main]
0 views
Skip to first unread message
Ayumi Ono (Gerrit)
Jan 13, 2026, 11:44:02 PM (4 hours ago) Jan 13
to Benoit Lize, Stephen Nusko, Chromium LUCI CQ, Eriko Kurimoto, Mikihito Matsuura, Rubber Stamper, AyeAye, luci-bi...@appspot.gserviceaccount.com, Code Review Nudger, chromium...@chromium.org, chrom...@appspot.gserviceaccount.com, Kentaro Hara
Attention needed from Benoit Lize, Eriko Kurimoto, Mikihito Matsuura and Stephen Nusko
Ayumi Ono has uploaded the change for review![]( "Open in Gerrit")
Ayumi Ono would like Benoit Lize, Stephen Nusko, Chromium LUCI CQ, Eriko Kurimoto, Mikihito Matsuura and Rubber Stamper to review this change.
Commit message
Reland "PA: Support multiple ThreadCache instances per thread."
This reverts commit 101ec6b775b79e6a644521d9213b3225237a1ee1.
Reason for revert: fixed
Original change's description:
> Revert "PA: Support multiple ThreadCache instances per thread."
>
> This reverts commit d7bfc3c111e238f13ccc81110cb8550cccb5a6a3.
>
> Reason for revert: Culprit of the wide-spread breakage of
> AlternateBucketDistributionAndPartitionFreeList*
>
> Example failure log:
> https://ci.chromium.org/ui/p/chromium/builders/ci/linux-chromeos-dbg/43949/overview
>
> Original change's description:
> > PA: Support multiple ThreadCache instances per thread.
> >
> > Currently, each thread only supports a single ThreadCache, which is
> > exclusively used by the default partition. This limitation hinders
> > security improvements that involve splitting the default partition into
> > several smaller partitions, as it leads to performance regressions due
> > to the lack of a thread cache for these new partitions.
> >
> > The design document is available in
> > https://docs.google.com/document/d/17wkGZ3ekQoirb0--kls-Sblr5qRSjtZ0VjtuJ5WIPH8/edit?usp=sharing
> > (internal only).
> >
> > This change introduces support for multiple `ThreadCache` instances per
> > thread. The number of thread caches is configured at compile time. These
> > caches are managed using:
> > - An array of `ThreadCache*` in `thread_local`.
> > - An array of `ThreadCache` within `PartitionTLS`.
> >
> > For `PartitionTLS`, an additional check ensures each `ThreadCache` is
> > properly initialized, as thread caches from different roots are created
> > at different times. The `IsTombstone` function has also been modified to
> > check against the entire thread's state rather than requiring a specific
> > `ThreadCache` pointer, since all thread caches for a given thread are
> > destroyed simultaneously.
> >
> > Note that additional testing, especially in `thread_cache_unittest.cc`,
> > will be added in a subsequent CL.
> >
> > Bug: 467243745
> > Change-Id: I217215f6db648ffba2f7313af2bdb546e36f6fe1
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7206958
> > Auto-Submit: Ayumi Ono <ayum...@google.com>
> > Reviewed-by: Benoit Lize <li...@chromium.org>
> > Commit-Queue: Ayumi Ono <ayum...@google.com>
> > Cr-Commit-Position: refs/heads/main@{#1568807}
>
> Bug: 467243745
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Change-Id: I8263c8be7b03694fefa6985609a230b62fc481a5
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7458168
> Commit-Queue: Rubber Stamper <rubber-...@appspot.gserviceaccount.com>
> Auto-Submit: Eriko Kurimoto <elk...@chromium.org>
> Owners-Override: Eriko Kurimoto <elk...@chromium.org>
> Bot-Commit: Rubber Stamper <rubber-...@appspot.gserviceaccount.com>
> Cr-Commit-Position: refs/heads/main@{#1568894}
Bug: 467243745
Change-Id: Idf77da44e59566ef83bb7a412f15b45f8ea43b8f
Change diff
Change information
Files:
- M base/allocator/partition_allocator/src/partition_alloc/extended_api.cc
- M base/allocator/partition_allocator/src/partition_alloc/memory_reclaimer_unittest.cc
- M base/allocator/partition_allocator/src/partition_alloc/partition_alloc.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/scheduler_loop_quarantine.cc
- M base/allocator/partition_allocator/src/partition_alloc/scheduler_loop_quarantine_support.cc
- M base/allocator/partition_allocator/src/partition_alloc/scheduler_loop_quarantine_support.h
- M base/allocator/partition_allocator/src/partition_alloc/scheduler_loop_quarantine_unittest.cc
- M base/allocator/partition_allocator/src/partition_alloc/thread_cache.cc
- M base/allocator/partition_allocator/src/partition_alloc/thread_cache.h
- M base/allocator/partition_allocator/src/partition_alloc/thread_cache_unittest.cc
- M base/trace_event/memory_infra_background_allowlist.cc
- M third_party/blink/renderer/platform/wtf/allocator/partitions.cc
Change size: L
Delta: 15 files changed, 322 insertions(+), 139 deletions(-)
Related details
Attention is currently required from:
- Benoit Lize
- Eriko Kurimoto
- Mikihito Matsuura
- Stephen Nusko
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rubber Stamper (Gerrit)
Jan 13, 2026, 11:44:22 PM (4 hours ago) Jan 13
to Ayumi Ono, Benoit Lize, Stephen Nusko, Chromium LUCI CQ, Eriko Kurimoto, Mikihito Matsuura, AyeAye, luci-bi...@appspot.gserviceaccount.com, Code Review Nudger, chromium...@chromium.org, chrom...@appspot.gserviceaccount.com, Kentaro Hara, kinuko...@chromium.org, lize...@chromium.org, wfh+...@chromium.org, bartek...@chromium.org, blink-...@chromium.org, tracing...@chromium.org, spang...@chromium.org, blink-re...@chromium.org, lizeb...@chromium.org
Attention needed from Benoit Lize, Eriko Kurimoto, Mikihito Matsuura and Stephen Nusko
Rubber Stamper voted Bot-Commit+1![]( "Open in Gerrit")
| Bot-Commit | +1 |
Related details
Attention is currently required from:
- Benoit Lize
- Eriko Kurimoto
- Mikihito Matsuura
- Stephen Nusko
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rubber Stamper (Gerrit)
Jan 13, 2026, 11:52:14 PM (3 hours ago) Jan 13
to Ayumi Ono, Benoit Lize, Stephen Nusko, Chromium LUCI CQ, Eriko Kurimoto, Mikihito Matsuura, AyeAye, luci-bi...@appspot.gserviceaccount.com, Code Review Nudger, chromium...@chromium.org, chrom...@appspot.gserviceaccount.com, Kentaro Hara, kinuko...@chromium.org, lize...@chromium.org, wfh+...@chromium.org, bartek...@chromium.org, blink-...@chromium.org, tracing...@chromium.org, spang...@chromium.org, blink-re...@chromium.org, lizeb...@chromium.org
Attention needed from Benoit Lize, Eriko Kurimoto, Mikihito Matsuura and Stephen Nusko
Message from Rubber Stamper![]( "Open in Gerrit")
Gerrit GetPureRevert API does not mark this CL as a pure revert. Learn more: go/rubber-stamper-user-guide.
Related details
Attention is currently required from:
- Benoit Lize
- Eriko Kurimoto
- Mikihito Matsuura
- Stephen Nusko
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mikihito Matsuura (Gerrit)
12:24 AM (3 hours ago) 12:24 AM
to Ayumi Ono, Benoit Lize, Stephen Nusko, Chromium LUCI CQ, Eriko Kurimoto, AyeAye, luci-bi...@appspot.gserviceaccount.com, Code Review Nudger, chromium...@chromium.org, chrom...@appspot.gserviceaccount.com, Kentaro Hara, kinuko...@chromium.org, lize...@chromium.org, wfh+...@chromium.org, bartek...@chromium.org, blink-...@chromium.org, tracing...@chromium.org, spang...@chromium.org, blink-re...@chromium.org, lizeb...@chromium.org
Attention needed from Ayumi Ono, Benoit Lize, Eriko Kurimoto and Stephen Nusko
Mikihito Matsuura voted Code-Review+1![]( "Open in Gerrit")
Attention is currently required from:
- Ayumi Ono
- Benoit Lize
- Eriko Kurimoto
- Stephen Nusko
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen Nusko (Gerrit)
12:29 AM (3 hours ago) 12:29 AM
to Ayumi Ono, Mikihito Matsuura, Benoit Lize, Chromium LUCI CQ, Eriko Kurimoto, AyeAye, luci-bi...@appspot.gserviceaccount.com, Code Review Nudger, chromium...@chromium.org, chrom...@appspot.gserviceaccount.com, Kentaro Hara, kinuko...@chromium.org, lize...@chromium.org, wfh+...@chromium.org, bartek...@chromium.org, blink-...@chromium.org, tracing...@chromium.org, spang...@chromium.org, blink-re...@chromium.org, lizeb...@chromium.org
Attention needed from Ayumi Ono, Benoit Lize and Eriko Kurimoto
Stephen Nusko voted and added 3 comments![]( "Open in Gerrit")
Votes added by Stephen Nusko
| Code-Review | +1 |
3 comments
Commit Message
Line 11, Patchset 4 (Latest):
Reason for revert: fixedStephen Nusko . unresolved
Can you expand on this? What did you change? Which patchset to diff to see the changes from the original?
File base/allocator/partition_allocator/src/partition_alloc/thread_cache_unittest.cc
Line 93, Patchset 4 (Latest):
opts.thread_cache_index = internal::kDefaultRootThreadCacheIndex;Stephen Nusko . unresolved
I think this is the change?
File third_party/blink/renderer/platform/wtf/allocator/partitions.cc
Line 147, Patchset 4 (Latest):
options.thread_cache_index = 0;Stephen Nusko . unresolved
This is just a magic number, could you either
1) Add a comment explaining why zero
2) Add a descriptively named variable set to zero and use that?
---------------------------
Optional further thought:
Also this is sort of hard for future users to know how to pick this value, obviously here they don't have PA-E, and the point is this is all known at compile time but perhaps we should have a `partition_alloc::GetNextAvailableThreadCacheIndex(partiton_alloc::ThreadCache::FastMalloc)`?
And then we explicitly enumerate all allowed usages and pass the correct values? This might make it be very clear what you need to do to add a new PartitionRoot with a thread cache.
Again this second half is completely optional, for this CL feel free to just add a comment about the zero or a named variable.
Related details
Attention is currently required from:
- Ayumi Ono
- Benoit Lize
- Eriko Kurimoto
Submit Requirements:
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. |
Ayumi Ono (Gerrit)
12:53 AM (2 hours ago) 12:53 AM
to Stephen Nusko, Mikihito Matsuura, Benoit Lize, Chromium LUCI CQ, Eriko Kurimoto, AyeAye, luci-bi...@appspot.gserviceaccount.com, Code Review Nudger, chromium...@chromium.org, chrom...@appspot.gserviceaccount.com, Kentaro Hara, kinuko...@chromium.org, lize...@chromium.org, wfh+...@chromium.org, bartek...@chromium.org, blink-...@chromium.org, tracing...@chromium.org, spang...@chromium.org, blink-re...@chromium.org, lizeb...@chromium.org
Attention needed from Benoit Lize and Eriko Kurimoto
Ayumi Ono voted and added 3 comments![]( "Open in Gerrit")
Votes added by Ayumi Ono
| Commit-Queue | +1 |
Can you expand on this? What did you change? Which patchset to diff to see the changes from the original?
Ayumi Ono
Done
File base/allocator/partition_allocator/src/partition_alloc/thread_cache_unittest.cc
Line 93, Patchset 4:
opts.thread_cache_index = internal::kDefaultRootThreadCacheIndex;Stephen Nusko . resolved
I think this is the change?
Ayumi Ono
Yes
File third_party/blink/renderer/platform/wtf/allocator/partitions.cc
This is just a magic number, could you either
1) Add a comment explaining why zero
2) Add a descriptively named variable set to zero and use that?---------------------------
Optional further thought:
Also this is sort of hard for future users to know how to pick this value, obviously here they don't have PA-E, and the point is this is all known at compile time but perhaps we should have a `partition_alloc::GetNextAvailableThreadCacheIndex(partiton_alloc::ThreadCache::FastMalloc)`?
And then we explicitly enumerate all allowed usages and pass the correct values? This might make it be very clear what you need to do to add a new PartitionRoot with a thread cache.
Again this second half is completely optional, for this CL feel free to just add a comment about the zero or a named variable.
Ayumi Ono
I fixed to use `partition_alloc::internal::kDefaultRootThreadCacheIndex`.
About the second half, it makes sense, but I think it's not feasible because the thread index occupation is different per threads and time; the default partition's thread_cache is currently configured during initialization, and not always configured.
Related details
Attention is currently required from:
- Benoit Lize
- Eriko Kurimoto
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ayumi Ono (Gerrit)
1:53 AM (1 hour ago) 1:53 AM
to Takashi Sakamoto, Stephen Nusko, Mikihito Matsuura, Chromium LUCI CQ, Eriko Kurimoto, AyeAye, luci-bi...@appspot.gserviceaccount.com, Code Review Nudger, chromium...@chromium.org, chrom...@appspot.gserviceaccount.com, Kentaro Hara, kinuko...@chromium.org, lize...@chromium.org, wfh+...@chromium.org, bartek...@chromium.org, blink-...@chromium.org, tracing...@chromium.org, spang...@chromium.org, blink-re...@chromium.org, lizeb...@chromium.org
Attention needed from Eriko Kurimoto and Takashi Sakamoto
Ayumi Ono voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ayumi Ono
| Commit-Queue | +1 |
1 comment
Patchset-level comments
File-level comment, Patchset 8 (Latest):
Ayumi Ono . resolved
PTAL
Patch 1 was already approved in "PA: Support multiple ThreadCache instances per thread". Patch 2 and 5 add new changes.
Related details
Attention is currently required from:
- Eriko Kurimoto
- Takashi Sakamoto
Submit Requirements:
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)
1:55 AM (1 hour ago) 1:55 AM
to Ayumi Ono, Stephen Nusko, Mikihito Matsuura, Chromium LUCI CQ, Eriko Kurimoto, AyeAye, luci-bi...@appspot.gserviceaccount.com, Code Review Nudger, chromium...@chromium.org, chrom...@appspot.gserviceaccount.com, Kentaro Hara, kinuko...@chromium.org, lize...@chromium.org, wfh+...@chromium.org, bartek...@chromium.org, blink-...@chromium.org, tracing...@chromium.org, spang...@chromium.org, blink-re...@chromium.org, lizeb...@chromium.org
Attention needed from Ayumi Ono and Eriko Kurimoto
Takashi Sakamoto voted Code-Review+1![]( "Open in Gerrit")
Attention is currently required from:
- Ayumi Ono
- Eriko Kurimoto
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages