Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

src: pre-emptively fix tests broken by V8 CL #48671

Closed
wants to merge 2 commits into from

Conversation

kvakil
Copy link
Contributor

@kvakil kvakil commented Jul 6, 2023

I put up a CL to V8 to fix the fact that kEagerCompile was being
completely ignored when used as an input to CompileFunction. Turns
out actually enabling kEagerCompile breaks a bunch of tests. Oops.

To stop us from getting the broken tests when we update V8, (1)
stop using kEagerCompile and (2) cherry-pick the patch. The latter
is not strictly necessary but will hopefully stop us from introducing
new use-cases of kEagerCompile which would break once it's actually
fixed.

Manually tested this by compiling before & after this PR and
inspecting out/Release/gen/node_snapshot.cc for differences. There
were no differences in the code cache but some minor differences
in the snapshot (which makes sense since snapshotting is not
deterministic).

src: remove kEagerCompile for CompileFunction

It wasn't doing anything, and actually enabling it would cause some
tests to fail.

Refs: #48576

deps: V8: cherry-pick cb00db4dba6c

Original commit message:

[compiler] fix CompileFunction ignoring kEagerCompile

v8::ScriptCompiler::CompileFunction was ignoring kEagerCompile. Unlike
the other functions in v8::ScriptCompiler, it was not actually
propagating kEagerCompile to the parser. The newly updated test fails
without this change.

I did some archeology and found that this was commented out since the
original CL in https://crrev.com/c/980944.

As far as I know Node.js is the main consumer of this particular API.
This CL speeds up Node.js's overall startup time by ~13%.

Change-Id: Ifc3cd6653555194d46ca48db14f7ba7a4afe0053
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4571822
Commit-Queue: Marja Hölttä <marja@chromium.org>
Reviewed-by: Marja Hölttä <marja@chromium.org>
Cr-Commit-Position: refs/heads/main@{#87944}

Refs: v8/v8@cb00db4

It wasn't doing anything, and actually enabling it would cause some
tests to fail.

Refs: nodejs#48576
Original commit message:

    [compiler] fix CompileFunction ignoring kEagerCompile

    v8::ScriptCompiler::CompileFunction was ignoring kEagerCompile. Unlike
    the other functions in v8::ScriptCompiler, it was not actually
    propagating kEagerCompile to the parser. The newly updated test fails
    without this change.

    I did some archeology and found that this was commented out since the
    original CL in https://crrev.com/c/980944.

    As far as I know Node.js is the main consumer of this particular API.
    This CL speeds up Node.js's overall startup time by ~13%.

    Change-Id: Ifc3cd6653555194d46ca48db14f7ba7a4afe0053
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4571822
    Commit-Queue: Marja Hölttä <marja@chromium.org>
    Reviewed-by: Marja Hölttä <marja@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#87944}

Refs: v8/v8@cb00db4
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/gyp
  • @nodejs/startup
  • @nodejs/v8-update

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Jul 6, 2023
@kvakil kvakil added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 6, 2023
@kvakil kvakil requested a review from RaisinTen July 6, 2023 07:42
@kvakil kvakil changed the title src: pre-emptively fix tests broken by patch src: pre-emptively fix tests broken by V8 CL Jul 6, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 6, 2023
@nodejs-github-bot
Copy link
Collaborator

Copy link
Contributor

@RaisinTen RaisinTen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@kvakil kvakil added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 6, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@kvakil kvakil added the commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. label Jul 8, 2023
@targos targos added the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 10, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 10, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 0c8539e...8a725c7

nodejs-github-bot pushed a commit that referenced this pull request Jul 10, 2023
It wasn't doing anything, and actually enabling it would cause some
tests to fail.

Refs: #48576
PR-URL: #48671
Refs: v8/v8@cb00db4
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
nodejs-github-bot pushed a commit that referenced this pull request Jul 10, 2023
Original commit message:

    [compiler] fix CompileFunction ignoring kEagerCompile

    v8::ScriptCompiler::CompileFunction was ignoring kEagerCompile. Unlike
    the other functions in v8::ScriptCompiler, it was not actually
    propagating kEagerCompile to the parser. The newly updated test fails
    without this change.

    I did some archeology and found that this was commented out since the
    original CL in https://crrev.com/c/980944.

    As far as I know Node.js is the main consumer of this particular API.
    This CL speeds up Node.js's overall startup time by ~13%.

    Change-Id: Ifc3cd6653555194d46ca48db14f7ba7a4afe0053
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4571822
    Commit-Queue: Marja Hölttä <marja@chromium.org>
    Reviewed-by: Marja Hölttä <marja@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#87944}

Refs: v8/v8@cb00db4
PR-URL: #48671
Refs: #48576
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
RaisinTen added a commit to RaisinTen/node that referenced this pull request Jul 10, 2023
Refs: nodejs#48671
Signed-off-by: Darshan Sen <raisinten@gmail.com>
anonrig pushed a commit to anonrig/node that referenced this pull request Jul 11, 2023
It wasn't doing anything, and actually enabling it would cause some
tests to fail.

Refs: nodejs#48576
PR-URL: nodejs#48671
Refs: v8/v8@cb00db4
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
anonrig pushed a commit to anonrig/node that referenced this pull request Jul 11, 2023
Original commit message:

    [compiler] fix CompileFunction ignoring kEagerCompile

    v8::ScriptCompiler::CompileFunction was ignoring kEagerCompile. Unlike
    the other functions in v8::ScriptCompiler, it was not actually
    propagating kEagerCompile to the parser. The newly updated test fails
    without this change.

    I did some archeology and found that this was commented out since the
    original CL in https://crrev.com/c/980944.

    As far as I know Node.js is the main consumer of this particular API.
    This CL speeds up Node.js's overall startup time by ~13%.

    Change-Id: Ifc3cd6653555194d46ca48db14f7ba7a4afe0053
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4571822
    Commit-Queue: Marja Hölttä <marja@chromium.org>
    Reviewed-by: Marja Hölttä <marja@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#87944}

Refs: v8/v8@cb00db4
PR-URL: nodejs#48671
Refs: nodejs#48576
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
juanarbol pushed a commit that referenced this pull request Jul 13, 2023
It wasn't doing anything, and actually enabling it would cause some
tests to fail.

Refs: #48576
PR-URL: #48671
Refs: v8/v8@cb00db4
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
juanarbol pushed a commit that referenced this pull request Jul 13, 2023
Original commit message:

    [compiler] fix CompileFunction ignoring kEagerCompile

    v8::ScriptCompiler::CompileFunction was ignoring kEagerCompile. Unlike
    the other functions in v8::ScriptCompiler, it was not actually
    propagating kEagerCompile to the parser. The newly updated test fails
    without this change.

    I did some archeology and found that this was commented out since the
    original CL in https://crrev.com/c/980944.

    As far as I know Node.js is the main consumer of this particular API.
    This CL speeds up Node.js's overall startup time by ~13%.

    Change-Id: Ifc3cd6653555194d46ca48db14f7ba7a4afe0053
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4571822
    Commit-Queue: Marja Hölttä <marja@chromium.org>
    Reviewed-by: Marja Hölttä <marja@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#87944}

Refs: v8/v8@cb00db4
PR-URL: #48671
Refs: #48576
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@juanarbol juanarbol mentioned this pull request Jul 13, 2023
RaisinTen added a commit to RaisinTen/node that referenced this pull request Jul 21, 2023
Refs: nodejs#48671
Signed-off-by: Darshan Sen <raisinten@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
It wasn't doing anything, and actually enabling it would cause some
tests to fail.

Refs: nodejs#48576
PR-URL: nodejs#48671
Refs: v8/v8@cb00db4
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
Original commit message:

    [compiler] fix CompileFunction ignoring kEagerCompile

    v8::ScriptCompiler::CompileFunction was ignoring kEagerCompile. Unlike
    the other functions in v8::ScriptCompiler, it was not actually
    propagating kEagerCompile to the parser. The newly updated test fails
    without this change.

    I did some archeology and found that this was commented out since the
    original CL in https://crrev.com/c/980944.

    As far as I know Node.js is the main consumer of this particular API.
    This CL speeds up Node.js's overall startup time by ~13%.

    Change-Id: Ifc3cd6653555194d46ca48db14f7ba7a4afe0053
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4571822
    Commit-Queue: Marja Hölttä <marja@chromium.org>
    Reviewed-by: Marja Hölttä <marja@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#87944}

Refs: v8/v8@cb00db4
PR-URL: nodejs#48671
Refs: nodejs#48576
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
It wasn't doing anything, and actually enabling it would cause some
tests to fail.

Refs: nodejs#48576
PR-URL: nodejs#48671
Refs: v8/v8@cb00db4
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
Original commit message:

    [compiler] fix CompileFunction ignoring kEagerCompile

    v8::ScriptCompiler::CompileFunction was ignoring kEagerCompile. Unlike
    the other functions in v8::ScriptCompiler, it was not actually
    propagating kEagerCompile to the parser. The newly updated test fails
    without this change.

    I did some archeology and found that this was commented out since the
    original CL in https://crrev.com/c/980944.

    As far as I know Node.js is the main consumer of this particular API.
    This CL speeds up Node.js's overall startup time by ~13%.

    Change-Id: Ifc3cd6653555194d46ca48db14f7ba7a4afe0053
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4571822
    Commit-Queue: Marja Hölttä <marja@chromium.org>
    Reviewed-by: Marja Hölttä <marja@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#87944}

Refs: v8/v8@cb00db4
PR-URL: nodejs#48671
Refs: nodejs#48576
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
ruyadorno pushed a commit that referenced this pull request Sep 11, 2023
It wasn't doing anything, and actually enabling it would cause some
tests to fail.

Refs: #48576
PR-URL: #48671
Refs: v8/v8@cb00db4
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
ruyadorno pushed a commit that referenced this pull request Sep 11, 2023
Original commit message:

    [compiler] fix CompileFunction ignoring kEagerCompile

    v8::ScriptCompiler::CompileFunction was ignoring kEagerCompile. Unlike
    the other functions in v8::ScriptCompiler, it was not actually
    propagating kEagerCompile to the parser. The newly updated test fails
    without this change.

    I did some archeology and found that this was commented out since the
    original CL in https://crrev.com/c/980944.

    As far as I know Node.js is the main consumer of this particular API.
    This CL speeds up Node.js's overall startup time by ~13%.

    Change-Id: Ifc3cd6653555194d46ca48db14f7ba7a4afe0053
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4571822
    Commit-Queue: Marja Hölttä <marja@chromium.org>
    Reviewed-by: Marja Hölttä <marja@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#87944}

Refs: v8/v8@cb00db4
PR-URL: #48671
Refs: #48576
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@ruyadorno ruyadorno mentioned this pull request Sep 11, 2023
ruyadorno pushed a commit that referenced this pull request Sep 13, 2023
It wasn't doing anything, and actually enabling it would cause some
tests to fail.

Refs: #48576
PR-URL: #48671
Refs: v8/v8@cb00db4
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
ruyadorno pushed a commit that referenced this pull request Sep 13, 2023
Original commit message:

    [compiler] fix CompileFunction ignoring kEagerCompile

    v8::ScriptCompiler::CompileFunction was ignoring kEagerCompile. Unlike
    the other functions in v8::ScriptCompiler, it was not actually
    propagating kEagerCompile to the parser. The newly updated test fails
    without this change.

    I did some archeology and found that this was commented out since the
    original CL in https://crrev.com/c/980944.

    As far as I know Node.js is the main consumer of this particular API.
    This CL speeds up Node.js's overall startup time by ~13%.

    Change-Id: Ifc3cd6653555194d46ca48db14f7ba7a4afe0053
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4571822
    Commit-Queue: Marja Hölttä <marja@chromium.org>
    Reviewed-by: Marja Hölttä <marja@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#87944}

Refs: v8/v8@cb00db4
PR-URL: #48671
Refs: #48576
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
ruyadorno pushed a commit that referenced this pull request Sep 17, 2023
It wasn't doing anything, and actually enabling it would cause some
tests to fail.

Refs: #48576
PR-URL: #48671
Refs: v8/v8@cb00db4
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
ruyadorno pushed a commit that referenced this pull request Sep 17, 2023
Original commit message:

    [compiler] fix CompileFunction ignoring kEagerCompile

    v8::ScriptCompiler::CompileFunction was ignoring kEagerCompile. Unlike
    the other functions in v8::ScriptCompiler, it was not actually
    propagating kEagerCompile to the parser. The newly updated test fails
    without this change.

    I did some archeology and found that this was commented out since the
    original CL in https://crrev.com/c/980944.

    As far as I know Node.js is the main consumer of this particular API.
    This CL speeds up Node.js's overall startup time by ~13%.

    Change-Id: Ifc3cd6653555194d46ca48db14f7ba7a4afe0053
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4571822
    Commit-Queue: Marja Hölttä <marja@chromium.org>
    Reviewed-by: Marja Hölttä <marja@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#87944}

Refs: v8/v8@cb00db4
PR-URL: #48671
Refs: #48576
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants