[PartitionAlloc] Dynamically adjust thread cache limits. [chromium/src : master]

Benoit L (Gerrit)

unread,

Mar 1, 2021, 1:40:55 PM3/1/21

to Kentaro Hara, kouhe...@chromium.org, lizeb...@chromium.org, oilpan-...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org

Attention is currently required from: Kentaro Hara.

Benoit L would like Kentaro Hara to review this change.

View Change

[PartitionAlloc] Dynamically adjust thread cache limits.

The thread cache keeps a number of deallocated objects, hoping to
service other allocations with it. This creates a compromise between
memory and performance.

This CL adds dynamic tuning of the thread cache limits. It can be set
process-wide, which will be used in subsequent CLs to adjust it
depending on the platform and/or process state
(e.g. foreground/background),

Bug: 998048, 1169159, 1169157
Change-Id: Ie7003fecd362ca613ea32d2242dbe811ea94246f
---
M base/allocator/partition_allocator/thread_cache.cc
M base/allocator/partition_allocator/thread_cache.h
M base/allocator/partition_allocator/thread_cache_unittest.cc
3 files changed, 231 insertions(+), 33 deletions(-)

To view, visit change 2727815. To unsubscribe, or for help writing mail filters, visit settings.

Benoit L (Gerrit)

unread,

Mar 1, 2021, 1:41:12 PM3/1/21

to kouhe...@chromium.org, lizeb...@chromium.org, oilpan-...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Kentaro Hara, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Kentaro Hara.

View Change

To view, visit change 2727815. To unsubscribe, or for help writing mail filters, visit settings.

Kentaro Hara (Gerrit)

unread,

Mar 2, 2021, 12:00:24 AM3/2/21

to Benoit L, kouhe...@chromium.org, lizeb...@chromium.org, oilpan-...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Benoit L.

Patch set 2:Code-Review +1

View Change

4 comments:

Patchset:
- Patch Set #2:
  I personally don't see a lot of benefits in making the threshold dynamically configurable but CrOS people might want it. LGTM.
File base/allocator/partition_allocator/thread_cache.h:
- Patch Set #2, Line 328: // value, just want to it not change while we are using it, hence using
  want it to not
File base/allocator/partition_allocator/thread_cache.cc:
- Patch Set #2, Line 183: // we really wnt to avoid atomic instruction on the fast path.
  want
- Patch Set #2, Line 311: value = initial_value / 4;
  Nit: I'm even fine with setting value to 0 for 512+ KB slots though. It costs a lot...

To view, visit change 2727815. To unsubscribe, or for help writing mail filters, visit settings.

Chromium LUCI CQ (Gerrit)

unread,

Mar 2, 2021, 7:43:32 AM3/2/21

to Benoit L, kouhe...@chromium.org, lizeb...@chromium.org, oilpan-...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Kentaro Hara, chromium...@chromium.org

Attention is currently required from: Kentaro Hara.

CQ is trying the patch.

Note: The patchset #3 "Typo" sent to CQ was uploaded after this CL was CR+1-ed.
Reviewer, please verify there is nothing unexpected https://chromium-review.googlesource.com/c/2727815/3

Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/2727815/3

Bot data: {"action": "start", "triggered_at": "2021-03-02T12:43:25.0Z", "revision": "48f672e6241d54d2e6ce612e529fd9aa1bb9c6dd"}

View Change

To view, visit change 2727815. To unsubscribe, or for help writing mail filters, visit settings.

Benoit L (Gerrit)

unread,

Mar 2, 2021, 7:43:44 AM3/2/21

to kouhe...@chromium.org, lizeb...@chromium.org, oilpan-...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Kentaro Hara, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Kentaro Hara.

Patch set 3:Commit-Queue +2

View Change

4 comments:

Patchset:
- Patch Set #3:
  Thanks!
File base/allocator/partition_allocator/thread_cache.h:

- Patch Set #2, Line 328: // value, just want to it not change while we are using it, hence using
  want it to not

- Done
File base/allocator/partition_allocator/thread_cache.cc:

- Patch Set #2, Line 183: // we really wnt to avoid atomic instruction on the fast path.
  want

- Done

- Patch Set #2, Line 311: value = initial_value / 4;
  Nit: I'm even fine with setting value to 0 for 512+ KB slots though. It costs a lot...

- It is 0 for allocations larger than 512 bytes, not kiB (ThreadCache::kSizeThreshold). One goal here is also to increase the current 512 bytes limit to a larger one, but only for foreground tabs for instance.
  But this would be in another CL, since it requires more changes.

To view, visit change 2727815. To unsubscribe, or for help writing mail filters, visit settings.

Chromium LUCI CQ (Gerrit)

unread,

Mar 2, 2021, 7:46:02 AM3/2/21

to Benoit L, kouhe...@chromium.org, lizeb...@chromium.org, oilpan-...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Kentaro Hara, chromium...@chromium.org

Chromium LUCI CQ submitted this change.

View Change

Approvals:
  Kentaro Hara: Looks good to me
  Benoit L: Commit

[PartitionAlloc] Dynamically adjust thread cache limits.

The thread cache keeps a number of deallocated objects, hoping to
service other allocations with it. This creates a compromise between
memory and performance.

This CL adds dynamic tuning of the thread cache limits. It can be set
process-wide, which will be used in subsequent CLs to adjust it
depending on the platform and/or process state
(e.g. foreground/background),

Bug: 998048, 1169159, 1169157
Change-Id: Ie7003fecd362ca613ea32d2242dbe811ea94246f

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2727815
Commit-Queue: Benoit L <li...@chromium.org>
Reviewed-by: Kentaro Hara <har...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#858966}


---
M base/allocator/partition_allocator/thread_cache.cc
M base/allocator/partition_allocator/thread_cache.h
M base/allocator/partition_allocator/thread_cache_unittest.cc
3 files changed, 231 insertions(+), 33 deletions(-)

2 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:

The name of the file: base/allocator/partition_allocator/thread_cache.cc
Insertions: 1, Deletions: 1.

```
@@ -182:183, +182:183 @@
-          // we really wnt to avoid atomic instruction on the fast path.
+          // we really want to avoid atomic instructions on the fast path.

```
The name of the file: base/allocator/partition_allocator/thread_cache.h
Insertions: 1, Deletions: 1.

```
@@ -327:328, +327:328 @@
-    // value, just want to it not change while we are using it, hence using
+    // value, just want it to not change while we are using it, hence using

```

To view, visit change 2727815. To unsubscribe, or for help writing mail filters, visit settings.

Reply all

Reply to author

Forward