GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — src: pre-emptively fix tests broken by V8 CL by kvakil · Pull Request #48671 · nodejs/node

Conversation

kvakil commented Jul 6, 2023

Copy link

Copy Markdown

Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bricss reacted with thumbs up emoji

All reactions

👍 1 reaction

kvakil added 2 commits

July 6, 2023 07:30


          src: remove kEagerCompile for CompileFunction

81a2cfe

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

Refs: nodejs#48576


          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 commented Jul 6, 2023

Copy link

Copy Markdown

Collaborator

Review requested:

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

nodejs-github-bot added lib / src

Issues and PRs involving general changes in the lib/ or src/ directories.

needs-ci PRs that need a full CI run. labels

Jul 6, 2023

kvakil added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

Jul 6, 2023

kvakil requested a review from RaisinTen

July 6, 2023 07:42

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 Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

Jul 6, 2023

nodejs-github-bot commented Jul 6, 2023

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

RaisinTen approved these changes

Jul 6, 2023

View reviewed changes

RaisinTen left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM, thanks!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

kvakil added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label

Jul 6, 2023

nodejs-github-bot commented Jul 6, 2023

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

nodejs-github-bot commented Jul 6, 2023

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

nodejs-github-bot commented Jul 6, 2023

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

kvakil added the commit-queue-rebase PRs the Commit Queue should land as multiple self-contained commits. label

Jul 8, 2023

RaisinTen mentioned this pull request

Jul 10, 2023

deps: update V8 to 11.6 #48710

Closed

targos approved these changes

Jul 10, 2023

View reviewed changes

targos added the commit-queue PRs queued for automated landing through the Commit Queue. label

Jul 10, 2023

nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label

Jul 10, 2023

nodejs-github-bot commented Jul 10, 2023

Copy link

Copy Markdown

Collaborator

Landed in 0c8539e...8a725c7

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

nodejs-github-bot closed this

Jul 10, 2023

nodejs-github-bot pushed a commit that referenced this pull request

Jul 10, 2023


          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


          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]>

juanarbol pushed a commit that referenced this pull request

Jul 13, 2023


          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


          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 mentioned this pull request

Jul 13, 2023

V20.5.0 proposal #48761

Merged

RaisinTen added a commit to RaisinTen/node that referenced this pull request

Jul 21, 2023


          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


          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


          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


          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


          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
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


          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


          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 mentioned this pull request

Sep 11, 2023

v18.18.0 proposal #49220

Merged

ruyadorno pushed a commit that referenced this pull request

Sep 13, 2023


          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


          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


          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


          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]>

Reviewers

RaisinTen

RaisinTen approved these changes

+1 more reviewer

targos

targos approved these changes

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready

PRs with CI started, the required approvals, and no outstanding review comments.

commit-queue-rebase

PRs the Commit Queue should land as multiple self-contained commits.

lib / src

Issues and PRs involving general changes in the lib/ or src/ directories.

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.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.