[PA][BRP] Disable PCScan in fake BRP experiment as well [chromium/src : main]

Bartek Nowierski (Gerrit)

unread,

Jun 16, 2021, 7:39:46 AM6/16/21

to Keishi Hattori, Sébastien Marchand, asvitki...@chromium.org, chromiumme...@microsoft.com, kouhe...@chromium.org, lizeb...@chromium.org, oilpan-...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org

Attention is currently required from: Keishi Hattori, Sébastien Marchand.

Bartek Nowierski would like Keishi Hattori and Sébastien Marchand to review this change.

View Change

[PA][BRP] Disable PCScan in fake BRP experiment as well

When testing BRP and PCScan together in Dev, we use this setup:
- 33.3% Binary A - BRP enabled
- 66.6% Binary B - BRP disabled, which is further split using Finch
  - 33.3% B1 - PCScan disabled
  - 33.3% B2 - PCScan enabled

Chromium code automatically disables PCScan when BRP is enabled, so
that comparing A vs. B1 only takes into account BRP and is unaffected by
PCScan.

The problem was that for the fake experiment, PCScan wasn't disabled in
binary A, so binary A and B1 didn't have identical behavior. This CL
fixes that.

Change-Id: Ia8af83c4a8676de40869a072bfdfaaa0a6d5d089
---
M base/allocator/partition_allocator/partition_alloc_config.h
M chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc
2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/base/allocator/partition_allocator/partition_alloc_config.h b/base/allocator/partition_allocator/partition_alloc_config.h
index 6d15bfd..8f3f786 100644
--- a/base/allocator/partition_allocator/partition_alloc_config.h
+++ b/base/allocator/partition_allocator/partition_alloc_config.h
@@ -22,7 +22,10 @@
 
 // BackupRefPtr and PCScan are incompatible, and due to its conservative nature,
 // it is 64 bits only.
-#if defined(PA_HAS_64_BITS_POINTERS) && !BUILDFLAG(USE_BACKUP_REF_PTR)
+// Disable PCScan even for USE_BACKUP_REF_PTR_FAKE, so that a "fake" BRP
+// experiment is unaffected by PCScan, as a non-fake one would.
+#if defined(PA_HAS_64_BITS_POINTERS) && !BUILDFLAG(USE_BACKUP_REF_PTR) && \
+    !BUILDFLAG(USE_BACKUP_REF_PTR_FAKE)
 #define PA_ALLOW_PCSCAN
 #endif
 
diff --git a/chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc b/chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc
index 133ee32..2e49699 100644
--- a/chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc
+++ b/chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc
@@ -660,7 +660,8 @@
   //    Update to fraction X of the non-enterprise population.
   //    Note, USE_BACKUP_REF_PTR_FAKE is only used to fake that the feature is
   //    enabled for the purpose of this Finch setting, while in fact there are
-  //    no behavior changes.
+  //    no behavior changes. Note, however, PCScan will be kept away from the
+  //    fake experiment binary, just as it would be from a regular one.
   // 3) The control group is established in fraction X of non-enterprise
   //    popluation via Finch (PartitionAllocBackupRefPtrControl). Since this
   //    Finch is applicable only to 1-X of the non-enterprise population, we

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

Bartek Nowierski (Gerrit)

unread,

Jun 16, 2021, 7:39:51 AM6/16/21

to asvitki...@chromium.org, chromiumme...@microsoft.com, kouhe...@chromium.org, lizeb...@chromium.org, oilpan-...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Keishi Hattori, Sébastien Marchand, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Keishi Hattori, Sébastien Marchand.

Patch set 1:Commit-Queue +1

View Change

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

Sébastien Marchand (Gerrit)

unread,

Jun 16, 2021, 10:00:19 AM6/16/21

to Bartek Nowierski, asvitki...@chromium.org, chromiumme...@microsoft.com, kouhe...@chromium.org, lizeb...@chromium.org, oilpan-...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Chromium LUCI CQ, Keishi Hattori, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Keishi Hattori, Bartek Nowierski.

Patch set 1:Code-Review +1

View Change

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

Bartek Nowierski (Gerrit)

unread,

Jun 17, 2021, 9:02:27 PM6/17/21

to Weilun Shi, asvitki...@chromium.org, chromiumme...@microsoft.com, kouhe...@chromium.org, lizeb...@chromium.org, oilpan-...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Keishi Hattori, Sébastien Marchand

Attention is currently required from: Keishi Hattori, Weilun Shi.

Bartek Nowierski would like Weilun Shi to review this change.

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

Bartek Nowierski (Gerrit)

unread,

Jun 17, 2021, 9:02:35 PM6/17/21

to asvitki...@chromium.org, chromiumme...@microsoft.com, kouhe...@chromium.org, lizeb...@chromium.org, oilpan-...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Weilun Shi, Keishi Hattori, Sébastien Marchand, Chromium LUCI CQ, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Keishi Hattori, Weilun Shi.

Patch set 1:Auto-Submit +1Commit-Queue +1

View Change

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

Weilun Shi (Gerrit)

unread,

Jun 18, 2021, 2:05:23 PM6/18/21

to Bartek Nowierski, asvitki...@chromium.org, chromiumme...@microsoft.com, kouhe...@chromium.org, lizeb...@chromium.org, oilpan-...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Keishi Hattori, Sébastien Marchand, Chromium LUCI CQ, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Keishi Hattori, Bartek Nowierski.

Patch set 1:Code-Review +1Commit-Queue +2

View Change

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

Chromium LUCI CQ (Gerrit)

unread,

Jun 18, 2021, 2:09:00 PM6/18/21

to Bartek Nowierski, asvitki...@chromium.org, chromiumme...@microsoft.com, kouhe...@chromium.org, lizeb...@chromium.org, oilpan-...@chromium.org, vmpstr...@chromium.org, wfh+...@chromium.org, Weilun Shi, Keishi Hattori, Sébastien Marchand, chromium...@chromium.org, Kentaro Hara

Chromium LUCI CQ submitted this change.

View Change

Approvals:
  Sébastien Marchand: Looks good to me
  Weilun Shi: Looks good to me; Commit
  Bartek Nowierski: Send CL to CQ automatically after approval

[PA][BRP] Disable PCScan in fake BRP experiment as well

When testing BRP and PCScan together in Dev, we use this setup:
- 33.3% Binary A - BRP enabled
- 66.6% Binary B - BRP disabled, which is further split using Finch
  - 33.3% B1 - PCScan disabled
  - 33.3% B2 - PCScan enabled

Chromium code automatically disables PCScan when BRP is enabled, so
that comparing A vs. B1 only takes into account BRP and is unaffected by
PCScan.

The problem was that for the fake experiment, PCScan wasn't disabled in
binary A, so binary A and B1 didn't have identical behavior. This CL
fixes that.

Change-Id: Ia8af83c4a8676de40869a072bfdfaaa0a6d5d089

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2966165
Reviewed-by: Sébastien Marchand <sebma...@chromium.org>
Reviewed-by: Weilun Shi <swe...@chromium.org>
Auto-Submit: Bartek Nowierski <bar...@chromium.org>
Commit-Queue: Weilun Shi <swe...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#893890}


---
M base/allocator/partition_allocator/partition_alloc_config.h
M chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc
2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/base/allocator/partition_allocator/partition_alloc_config.h b/base/allocator/partition_allocator/partition_alloc_config.h
index 6d15bfd..8f3f786 100644
--- a/base/allocator/partition_allocator/partition_alloc_config.h
+++ b/base/allocator/partition_allocator/partition_alloc_config.h
@@ -22,7 +22,10 @@
 
 // BackupRefPtr and PCScan are incompatible, and due to its conservative nature,
 // it is 64 bits only.
-#if defined(PA_HAS_64_BITS_POINTERS) && !BUILDFLAG(USE_BACKUP_REF_PTR)
+// Disable PCScan even for USE_BACKUP_REF_PTR_FAKE, so that a "fake" BRP
+// experiment is unaffected by PCScan, as a non-fake one would.
+#if defined(PA_HAS_64_BITS_POINTERS) && !BUILDFLAG(USE_BACKUP_REF_PTR) && \
+    !BUILDFLAG(USE_BACKUP_REF_PTR_FAKE)
 #define PA_ALLOW_PCSCAN
 #endif
 
diff --git a/chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc b/chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc

index a46887e..0c1960e 100644
--- a/chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc
+++ b/chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc
@@ -665,7 +665,8 @@


   //    Update to fraction X of the non-enterprise population.
   //    Note, USE_BACKUP_REF_PTR_FAKE is only used to fake that the feature is
   //    enabled for the purpose of this Finch setting, while in fact there are
-  //    no behavior changes.
+  //    no behavior changes. Note, however, PCScan will be kept away from the
+  //    fake experiment binary, just as it would be from a regular one.
   // 3) The control group is established in fraction X of non-enterprise
   //    popluation via Finch (PartitionAllocBackupRefPtrControl). Since this
   //    Finch is applicable only to 1-X of the non-enterprise population, we

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

Reply all

Reply to author

Forward