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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 2 commits into from
Closed

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

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ä <[email protected]>
Reviewed-by: Marja Hölttä <[email protected]>
Cr-Commit-Position: refs/heads/main@{#87944}

Refs: v8/v8@cb00db4

@kvakil
src: remove kEagerCompile for CompileFunction
81a2cfe 
It wasn't doing anything, and actually enabling it would cause some
tests to fail.

Refs: nodejs#48576
@kvakil
deps: V8: cherry-pick cb00db4dba6c
64583df 
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ä <[email protected]>
    Reviewed-by: Marja Hölttä <[email protected]>
    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

CI: https://ci.nodejs.org/job/node-test-pull-request/52631/

RaisinTen
RaisinTen approved these changes Jul 6, 2023
View reviewed changes
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

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=benchmark-ubuntu2204-intel-64,v8test=v8test/5431/

@nodejs-github-bot
Copy link
Collaborator

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-s390x,v8test=v8test/5431/

@nodejs-github-bot
Copy link
Collaborator

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-ppc64le,v8test=v8test/5431/

@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
@RaisinTen RaisinTen mentioned this pull request Jul 10, 2023
Closed
@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
@kvakil @nodejs-github-bot
src: remove kEagerCompile for CompileFunction
f08341c 
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 <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
nodejs-github-bot pushed a commit that referenced this pull request Jul 10, 2023
@kvakil @nodejs-github-bot
deps: V8: cherry-pick cb00db4dba6c
8a725c7 
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ä <[email protected]>
    Reviewed-by: Marja Hölttä <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#87944}

Refs: v8/v8@cb00db4
PR-URL: #48671
Refs: #48576
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
RaisinTen added a commit to RaisinTen/node that referenced this pull request Jul 10, 2023
@RaisinTen
src: use kNoCompileOptions instead of kEagerCompile
c758192 
Refs: nodejs#48671
Signed-off-by: Darshan Sen <[email protected]>
anonrig pushed a commit to anonrig/node that referenced this pull request Jul 11, 2023
@kvakil @anonrig
src: remove kEagerCompile for CompileFunction
08459f5 
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 <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
anonrig pushed a commit to anonrig/node that referenced this pull request Jul 11, 2023
@kvakil @anonrig
deps: V8: cherry-pick cb00db4dba6c
f4595bd 
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ä <[email protected]>
    Reviewed-by: Marja Hölttä <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#87944}

Refs: v8/v8@cb00db4
PR-URL: nodejs#48671
Refs: nodejs#48576
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
juanarbol pushed a commit that referenced this pull request Jul 13, 2023
@kvakil @juanarbol
src: remove kEagerCompile for CompileFunction
d99ea48 
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 <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
juanarbol pushed a commit that referenced this pull request Jul 13, 2023
@kvakil @juanarbol
deps: V8: cherry-pick cb00db4dba6c
b7a076a 
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ä <[email protected]>
    Reviewed-by: Marja Hölttä <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#87944}

Refs: v8/v8@cb00db4
PR-URL: #48671
Refs: #48576
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@juanarbol juanarbol mentioned this pull request Jul 13, 2023
Merged
RaisinTen added a commit to RaisinTen/node that referenced this pull request Jul 21, 2023
@RaisinTen
src: use kNoCompileOptions instead of kEagerCompile
1f8f86b 
Refs: nodejs#48671
Signed-off-by: Darshan Sen <[email protected]>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@kvakil @Ceres6
src: remove kEagerCompile for CompileFunction
0b1f4d4 
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 <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@kvakil @Ceres6
deps: V8: cherry-pick cb00db4dba6c
eb40e29 
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ä <[email protected]>
    Reviewed-by: Marja Hölttä <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#87944}

Refs: v8/v8@cb00db4
PR-URL: nodejs#48671
Refs: nodejs#48576
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@kvakil @Ceres6
src: remove kEagerCompile for CompileFunction
3a1da18 
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 <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@kvakil @Ceres6
deps: V8: cherry-pick cb00db4dba6c
6138776 
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ä <[email protected]>
    Reviewed-by: Marja Hölttä <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#87944}

Refs: v8/v8@cb00db4
PR-URL: nodejs#48671
Refs: nodejs#48576
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
ruyadorno pushed a commit that referenced this pull request Sep 11, 2023
@kvakil @ruyadorno
src: remove kEagerCompile for CompileFunction
ae11d1e 
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 <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
ruyadorno pushed a commit that referenced this pull request Sep 11, 2023
@kvakil @ruyadorno
deps: V8: cherry-pick cb00db4dba6c
4af650c 
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ä <[email protected]>
    Reviewed-by: Marja Hölttä <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#87944}

Refs: v8/v8@cb00db4
PR-URL: #48671
Refs: #48576
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@ruyadorno ruyadorno mentioned this pull request Sep 11, 2023
Merged
ruyadorno pushed a commit that referenced this pull request Sep 13, 2023
@kvakil @ruyadorno
src: remove kEagerCompile for CompileFunction
0de99d2 
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 <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
ruyadorno pushed a commit that referenced this pull request Sep 13, 2023
@kvakil @ruyadorno
deps: V8: cherry-pick cb00db4dba6c
222082d 
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ä <[email protected]>
    Reviewed-by: Marja Hölttä <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#87944}

Refs: v8/v8@cb00db4
PR-URL: #48671
Refs: #48576
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
ruyadorno pushed a commit that referenced this pull request Sep 17, 2023
@kvakil @ruyadorno
src: remove kEagerCompile for CompileFunction
3f65598 
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 <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
ruyadorno pushed a commit that referenced this pull request Sep 17, 2023
@kvakil @ruyadorno
deps: V8: cherry-pick cb00db4dba6c
bcb255d 
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ä <[email protected]>
    Reviewed-by: Marja Hölttä <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#87944}

Refs: v8/v8@cb00db4
PR-URL: #48671
Refs: #48576
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RaisinTen RaisinTen RaisinTen approved these changes

@targos targos targos approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kvakil @nodejs-github-bot @targos @RaisinTen