Turning off use_v8_context_snapshot in non-production builds by default

[Nico Weber]

Hello,

use_v8_context_snapshot causes us to build lots of files (blink + dependencies) twice on bots that cross-compiler (win/arm64, android, etc).

We'd like to turn off use_v8_context_snapshot in regular release builds by default, and only keep it enabled in is_official_build builds.

However, a small number of tests, almost all of them inspector-protocol tests, fail if I try this: https://chromium-review.googlesource.com/c/chromium/src/+/5704136

That's surprising to me. I would've expected use_v8_context_snapshot to not change behavior.

Does anyone have an idea why this might happen?

Thanks,

Nico

[Dave Tapuska]

Have you thought about setting gn config v8_use_external_startup_data on any bots?

I don't know why they are failing. The window constructor one is weird as well because the prototype chain is different...

dave.

--
You received this message because you are subscribed to the Google Groups "blink-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CADZ1XibPV%3Dd448fckQeYFaO617_9aahsxysAjMEdQ4sy%3DQw4kQ%40mail.gmail.com.

[Leszek Swirski]

+Michael Lippautz 

I'm not sure we should do this for build time reasons alone. `use_v8_context_snapshot` is the default behaviour in shipping browsers, and it makes the renderer take quite a different path during page load / navigation. If we disable it in regular builds, then regular builds (including all the dchecks!) won't be running and texting the same code that we're shipping.

The fact that tests are failing with this flag change should be a huge flashing red light about this proposal - if we make the tests not test the code we're shipping, then we risk it breaking without us noticing. Indeed, I've been trying to turn this flag on for Android, and have been struggling to do so.

- Leszek

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAHgVhZU-er_dfh_esgBmfLQ%2BOVf24y2yN7K6W6yjGqNkH%3Df%3D6w%40mail.gmail.com.

[Nico Weber]

If the code path behind use_v8_context_snapshot is subtle and hard to get right, it sounds like there's our second reason for not having it, right there :)

How much performance does the blink snapshot buy us? Is it possible to get some of that with less expensive techniques?

(+sky who I think looked at this at some point.)

On Monday, July 15, 2024 at 2:54:35 PM UTC-4 Leszek Swirski wrote:

+Michael Lippautz 

I'm not sure we should do this for build time reasons alone. `use_v8_context_snapshot` is the default behaviour in shipping browsers, and it makes the renderer take quite a different path during page load / navigation. If we disable it in regular builds, then regular builds (including all the dchecks!) won't be running and texting the same code that we're shipping.

The fact that tests are failing with this flag change should be a huge flashing red light about this proposal - if we make the tests not test the code we're shipping, then we risk it breaking without us noticing. Indeed, I've been trying to turn this flag on for Android, and have been struggling to do so.

- Leszek

On Mon, 15 Jul 2024, 20:25 Dave Tapuska, <dtap...@chromium.org> wrote:

Have you thought about setting gn config v8_use_external_startup_data on any bots?

I don't know why they are failing. The window constructor one is weird as well because the prototype chain is different...

dave.

On Mon, Jul 15, 2024 at 2:13 PM Nico Weber <tha...@chromium.org> wrote:

Hello,

use_v8_context_snapshot causes us to build lots of files (blink + dependencies) twice on bots that cross-compiler (win/arm64, android, etc).

We'd like to turn off use_v8_context_snapshot in regular release builds by default, and only keep it enabled in is_official_build builds.

However, a small number of tests, almost all of them inspector-protocol tests, fail if I try this: https://chromium-review.googlesource.com/c/chromium/src/+/5704136

That's surprising to me. I would've expected use_v8_context_snapshot to not change behavior.

Does anyone have an idea why this might happen?

Thanks,

Nico

--
You received this message because you are subscribed to the Google Groups "blink-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+unsubscribe@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CADZ1XibPV%3Dd448fckQeYFaO617_9aahsxysAjMEdQ4sy%3DQw4kQ%40mail.gmail.com.

--
You received this message because you are subscribed to the Google Groups "blink-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+unsubscribe@chromium.org.

[Leszek Swirski]

I'm not saying that it's subtle and hard to get right, any more so than any other code we ship at least, I'm saying that it's a full separate code path and behaviour that we would be shipping to users without running locally. The difficulties I'm having on android are because of build config/dependency issues. 

For "how much performance", I'm currently trying to enable it on Android because it would improve Search page load by what they estimate to be ~24 SWE-years worth of improvement (Kentaro has a calculation in this doc). Also +Kouhei Ueno who has been looking into the impact of context setup on page load, and +Camillo Bruni who looked at the performance with Scott.

On Tue, 16 Jul 2024, 02:43 Nico Weber, <tha...@chromium.org> wrote:

If the code path behind use_v8_context_snapshot is subtle and hard to get right, it sounds like there's our second reason for not having it, right there :)

How much performance does the blink snapshot buy us? Is it possible to get some of that with less expensive techniques?

(+sky who I think looked at this at some point.)

On Monday, July 15, 2024 at 2:54:35 PM UTC-4 Leszek Swirski wrote:

+Michael Lippautz 

I'm not sure we should do this for build time reasons alone. `use_v8_context_snapshot` is the default behaviour in shipping browsers, and it makes the renderer take quite a different path during page load / navigation. If we disable it in regular builds, then regular builds (including all the dchecks!) won't be running and texting the same code that we're shipping.

The fact that tests are failing with this flag change should be a huge flashing red light about this proposal - if we make the tests not test the code we're shipping, then we risk it breaking without us noticing. Indeed, I've been trying to turn this flag on for Android, and have been struggling to do so.

- Leszek

On Mon, 15 Jul 2024, 20:25 Dave Tapuska, <dtap...@chromium.org> wrote:

Have you thought about setting gn config v8_use_external_startup_data on any bots?

I don't know why they are failing. The window constructor one is weird as well because the prototype chain is different...

dave.

On Mon, Jul 15, 2024 at 2:13 PM Nico Weber <tha...@chromium.org> wrote:

Hello,

use_v8_context_snapshot causes us to build lots of files (blink + dependencies) twice on bots that cross-compiler (win/arm64, android, etc).

We'd like to turn off use_v8_context_snapshot in regular release builds by default, and only keep it enabled in is_official_build builds.

However, a small number of tests, almost all of them inspector-protocol tests, fail if I try this: https://chromium-review.googlesource.com/c/chromium/src/+/5704136

That's surprising to me. I would've expected use_v8_context_snapshot to not change behavior.

Does anyone have an idea why this might happen?

Thanks,

Nico

--
You received this message because you are subscribed to the Google Groups "blink-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CADZ1XibPV%3Dd448fckQeYFaO617_9aahsxysAjMEdQ4sy%3DQw4kQ%40mail.gmail.com.

--
You received this message because you are subscribed to the Google Groups "blink-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+...@chromium.org.

[Daniel Cheng]

I'm guessing the root cause is this:

--- /b/s/w/ioajsmeisv/layout-test-results/fast/dom/Window/window-constructor-expected.txt
+++ /b/s/w/ioajsmeisv/layout-test-results/fast/dom/Window/window-constructor-actual.txt
@@ -0,0 +1,5 @@
+This is a testharness.js-based test.
+[FAIL] Test Window and its prototype chain's constructors.
+  assert_equals: WindowProperties constructor is EventTarget. expected function "function EventTarget() { [native code] }" but got function "function WindowProperties() { [native code] }"
+Harness: the test ran to completion.
+

Though *why* it's failing, I don't know off hand and will need to dig into it. It does seem worth fixing that.

Daniel

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAGRskv_WMBbxoTeVLhK4Pst0tSk6gOnC2ijy6vdH86P1y_bAZA%40mail.gmail.com.

[Michael Lippautz]

The context snapshot is the default behavior that we ship and we should test. If there's issues with tests, we should just fix them, like anything else. 

Using the snapshot exercises very different paths for something like e.g. bindings. IMHO, a removal should be treated like any other bigger removals: Someone comes up with a proposal and a Finch trial + reasons why it's not necessary anymore. Context snapshots are a performance feature in that they eagerly create objects that we then later need and as such benefit startup.

-Michael

[Scott Violet]

The v8 snapshots provide substantial performance gains. Leszek provided the data to back that up. We shouldn't remove them. The v8 team is in the best position to answer if there are other strategies that could give the same performance wins without having the additional build time.

While I believe we shouldn't remove the snapshots, I sympathize with the desire for faster builds. Seems like the thing we care about is ensuring we don't regress test coverage. So, if we were to disable the snapshots on many bots, we need to ensure we have at least one bot (on the main waterfall) that is building the snapshots.

  -Scott

On Mon, Jul 15, 2024 at 5:43 PM Nico Weber <tha...@chromium.org> wrote:

If the code path behind use_v8_context_snapshot is subtle and hard to get right, it sounds like there's our second reason for not having it, right there :)

How much performance does the blink snapshot buy us? Is it possible to get some of that with less expensive techniques?

(+sky who I think looked at this at some point.)

On Monday, July 15, 2024 at 2:54:35 PM UTC-4 Leszek Swirski wrote:

+Michael Lippautz 

I'm not sure we should do this for build time reasons alone. `use_v8_context_snapshot` is the default behaviour in shipping browsers, and it makes the renderer take quite a different path during page load / navigation. If we disable it in regular builds, then regular builds (including all the dchecks!) won't be running and texting the same code that we're shipping.

The fact that tests are failing with this flag change should be a huge flashing red light about this proposal - if we make the tests not test the code we're shipping, then we risk it breaking without us noticing. Indeed, I've been trying to turn this flag on for Android, and have been struggling to do so.

- Leszek

On Mon, 15 Jul 2024, 20:25 Dave Tapuska, <dtap...@chromium.org> wrote:

Have you thought about setting gn config v8_use_external_startup_data on any bots?

I don't know why they are failing. The window constructor one is weird as well because the prototype chain is different...

dave.

On Mon, Jul 15, 2024 at 2:13 PM Nico Weber <tha...@chromium.org> wrote:

Hello,

use_v8_context_snapshot causes us to build lots of files (blink + dependencies) twice on bots that cross-compiler (win/arm64, android, etc).

We'd like to turn off use_v8_context_snapshot in regular release builds by default, and only keep it enabled in is_official_build builds.

However, a small number of tests, almost all of them inspector-protocol tests, fail if I try this: https://chromium-review.googlesource.com/c/chromium/src/+/5704136

That's surprising to me. I would've expected use_v8_context_snapshot to not change behavior.

Does anyone have an idea why this might happen?

Thanks,

Nico

--
You received this message because you are subscribed to the Google Groups "blink-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CADZ1XibPV%3Dd448fckQeYFaO617_9aahsxysAjMEdQ4sy%3DQw4kQ%40mail.gmail.com.

--
You received this message because you are subscribed to the Google Groups "blink-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+...@chromium.org.

[Leszek Swirski]

We could also disable for, e.g., developer debug builds, but keep enabled on bots (there's some precedent in V8 for "fast snapshots" for local dev builds).

[Jeremy Roman]

If we do so we could also only do so in build configurations that are actually cross-compiling -- the build time overhead on, e.g., Linux x64 targeting Linux x64 is much less.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAGRskv-Nso4p3X-%3DEBPpi08h246gbjTRP8v7rVx%2BS3dH925%3D6A%40mail.gmail.com.

[Erik Staab]

Let me try to summarize this thread and clarify a few points to help guide us to a decision.

1) The snapshot provides a significant performance improvement and there are currently no alternate strategies that would be better for build times

2) We need test coverage for the snapshot code path since it's what we ship to users and behavior can differ

3) Some tests are producing different results with the snapshot on and off, which we should fix regardless of the decision

4) Generating the snapshot has a significant impact on build times / costs, especially when cross compiling

There are multiple lines of defense against shipping bugs to users in order of decreasing costs and increasing coverage:

a) local debug edit test cycles

b) CQ

c) Gardened CI builders

Right now we're paying a significant cost by enabling the snapshot everywhere. We could:

i) disable it for (a) and the only difference is a CL author may see an occasional CQ failure due to the difference between snapshot on and off behavior

ii) disable it for (a) and (b) and occasionally a CL will land and get reverted by gardeners

In both of these scenarios we're not shipping more bugs to users.

My suggestion would be to do (ii) and turn it off in critical paths for engineers knowing that we may need to revert a CL periodically. We should ensure that tests with both snapshot on and off code paths are healthy so that this is rare. I have a query to see the revert rate in chromium/src -- if we see more reverts due to this change then we can revisit and do (i) instead, or look into other options.

If we aren't shipping more bugs to users and we monitor the revert rate does anyone have a problem with turning off snapshots in the critical development path?

Thanks,

Erik

[Jeremy Roman]

On Wed, Jul 17, 2024 at 4:18 PM Erik Staab <est...@chromium.org> wrote:

Let me try to summarize this thread and clarify a few points to help guide us to a decision.

1) The snapshot provides a significant performance improvement and there are currently no alternate strategies that would be better for build times

2) We need test coverage for the snapshot code path since it's what we ship to users and behavior can differ

3) Some tests are producing different results with the snapshot on and off, which we should fix regardless of the decision

4) Generating the snapshot has a significant impact on build times / costs, especially when cross compiling

There are multiple lines of defense against shipping bugs to users in order of decreasing costs and increasing coverage:

a) local debug edit test cycles

b) CQ

c) Gardened CI builders

Right now we're paying a significant cost by enabling the snapshot everywhere. We could:

i) disable it for (a) and the only difference is a CL author may see an occasional CQ failure due to the difference between snapshot on and off behavior

ii) disable it for (a) and (b) and occasionally a CL will land and get reverted by gardeners

In both of these scenarios we're not shipping more bugs to users.

My suggestion would be to do (ii) and turn it off in critical paths for engineers knowing that we may need to revert a CL periodically. We should ensure that tests with both snapshot on and off code paths are healthy so that this is rare. I have a query to see the revert rate in chromium/src -- if we see more reverts due to this change then we can revisit and do (i) instead, or look into other options.

If we aren't shipping more bugs to users and we monitor the revert rate does anyone have a problem with turning off snapshots in the critical development path?

If the motivation mainly applies to cross-compiled configurations, why change native build configurations (which I think includes most desktop build configurations, at least) at all?

[Brian Osman]

I want to clarify something about the "slower builds" (I originally reported the issue that led to this investigation/discussion). I was specifically interested in CQ (not local) jobs, as it related to web test rebaselining. The standard workflow for generating new web test baselines involves running a suite of CQ jobs. Of those, the win11-arm64 builders take 50% (a full hour) longer than any other job in the suite. That's a major workflow impact, and not addressed by changes that only apply to local builds. It sounds like Jeremy's suggestion would address this, but I know there are other proposals elsewhere (eg, increasing the builder bot dimensions to account for the extra work imposed on the windows-arm jobs).

[Erik Staab]

> If the motivation mainly applies to cross-compiled configurations, why change native build configurations (which I think includes most desktop build configurations, at least) at all?

I think this would be fine since I don't *think* building the snapshot costs much when not cross-compiling. However, whether or not something cross-compiles on CQ is somewhat opaque and an implementation detail that could make debugging harder. For example, mac-rel builds on arm64 and targets x64, a change we made without a big announcement or anyone outside of infra needing to know at the time. Maybe we can surface whether we're using the snapshot or not in the logs?

Either way, I think we want to have explicit snapshot-on and snapshot-off builders in CI so we continue to have coverage fairly close to when CL lands but outside of the critical path.