Skip to content

Conversation

@Renegade334
Copy link
Member

Fixes: #60030

The ChoiceNode changes are built on a couple of changes to graph handling in 13.8 that need to come along as well; these are side-effect-free from an end-user perspective.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/gyp
  • @nodejs/security-wg
  • @nodejs/v8-update

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. v24.x Issues that can be reproduced on v24.x or PRs targeting the v24.x-staging branch. v8 engine Issues and PRs related to the V8 dependency. labels Nov 15, 2025
@Renegade334 Renegade334 requested a review from targos November 15, 2025 21:27
Erik Corry and others added 4 commits December 6, 2025 17:16
Original commit message:

    [regexp] Rename "greedy loops" to "fixed length".

    Some greedy quantifiers are code-generated in a more efficient
    way, but far from all greedy quantifiers are generated
    in this way.  This change renames the specially optimized loops
    from "greedy loops" to "fixed length loops", which should be
    clearer.

    Going forward, we can probably reuse much of the code for
    fixed length loops when code-generating simple possessive
    quantifiers.

    Change-Id: I13b9d14beac430e2d05d0feaf887fc0566bc4103
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/6508846
    Reviewed-by: Patrick Thier <pthier@chromium.org>
    Commit-Queue: Erik Corry <erikcorry@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#100062}

Refs: v8/v8@df20105
Original commit message:

    [regexp] Remove DeferredAction class.

    We can just chain up the traces and use the regular
    ActionNodes that we already have to represent the
    deferred actions.  Also simplifies the ActionNodes
    a little.  No functional change intended.

    Reduce the size of the Trace from 128->120 bytes.

    This is a stack allocated struct so to avoid stack
    overflows after adding the next_ field I am reducing
    it back down to 120 bytes by rearranging and shrinking
    fields.

    Change-Id: I6dca9946e035e9b22798e160b8fadaeca61f4955
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/6512931
    Reviewed-by: Patrick Thier <pthier@chromium.org>
    Commit-Queue: Erik Corry <erikcorry@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#100092}

Refs: v8/v8@0dd2318
Original commit message:

    [regexp] Clean up state for fixed length loop.

    Also reduces the on-stack Trace size by one word,
    adds some comments, renames some variables for
    more clarity.

    Change-Id: I9ec105cd9cebbaba65e9801c47dd0574cc81f967
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/6512896
    Reviewed-by: Patrick Thier <pthier@chromium.org>
    Commit-Queue: Erik Corry <erikcorry@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#100117}

Refs: v8/v8@6bb32bd
Original commit message:

    [regexp] Fix modifiers for ChoiceNodes

    Each alternative might modify flags when their sub-graph is emitted.
    We need to restore flags to the value at the beginning of a ChoiceNode
    for each alternative.

    Drive-by: Move regexp-modifiers test out of harmony/

    Fixed: 447583670
    Change-Id: I9f41e51f34df7659461da0a4fcd28b7e157f52e1
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/6995181
    Reviewed-by: Jakob Linke <jgruber@chromium.org>
    Commit-Queue: Patrick Thier <pthier@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#102838}

Refs: v8/v8@72b0e27
Fixes: nodejs#60030
@targos
Copy link
Member

targos commented Dec 6, 2025

Rebased.

@targos targos added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 6, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 6, 2025
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@RafaelGSS RafaelGSS left a comment

Choose a reason for hiding this comment

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

RSLGTM

@nodejs-github-bot
Copy link
Collaborator

targos pushed a commit that referenced this pull request Dec 7, 2025
Original commit message:

    [regexp] Rename "greedy loops" to "fixed length".

    Some greedy quantifiers are code-generated in a more efficient
    way, but far from all greedy quantifiers are generated
    in this way.  This change renames the specially optimized loops
    from "greedy loops" to "fixed length loops", which should be
    clearer.

    Going forward, we can probably reuse much of the code for
    fixed length loops when code-generating simple possessive
    quantifiers.

    Change-Id: I13b9d14beac430e2d05d0feaf887fc0566bc4103
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/6508846
    Reviewed-by: Patrick Thier <pthier@chromium.org>
    Commit-Queue: Erik Corry <erikcorry@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#100062}

Refs: v8/v8@df20105
PR-URL: #60732
Fixes: #60030
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
targos pushed a commit that referenced this pull request Dec 7, 2025
Original commit message:

    [regexp] Remove DeferredAction class.

    We can just chain up the traces and use the regular
    ActionNodes that we already have to represent the
    deferred actions.  Also simplifies the ActionNodes
    a little.  No functional change intended.

    Reduce the size of the Trace from 128->120 bytes.

    This is a stack allocated struct so to avoid stack
    overflows after adding the next_ field I am reducing
    it back down to 120 bytes by rearranging and shrinking
    fields.

    Change-Id: I6dca9946e035e9b22798e160b8fadaeca61f4955
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/6512931
    Reviewed-by: Patrick Thier <pthier@chromium.org>
    Commit-Queue: Erik Corry <erikcorry@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#100092}

Refs: v8/v8@0dd2318
PR-URL: #60732
Fixes: #60030
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
targos pushed a commit that referenced this pull request Dec 7, 2025
Original commit message:

    [regexp] Clean up state for fixed length loop.

    Also reduces the on-stack Trace size by one word,
    adds some comments, renames some variables for
    more clarity.

    Change-Id: I9ec105cd9cebbaba65e9801c47dd0574cc81f967
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/6512896
    Reviewed-by: Patrick Thier <pthier@chromium.org>
    Commit-Queue: Erik Corry <erikcorry@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#100117}

Refs: v8/v8@6bb32bd
PR-URL: #60732
Fixes: #60030
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
targos pushed a commit that referenced this pull request Dec 7, 2025
Original commit message:

    [regexp] Fix modifiers for ChoiceNodes

    Each alternative might modify flags when their sub-graph is emitted.
    We need to restore flags to the value at the beginning of a ChoiceNode
    for each alternative.

    Drive-by: Move regexp-modifiers test out of harmony/

    Fixed: 447583670
    Change-Id: I9f41e51f34df7659461da0a4fcd28b7e157f52e1
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/6995181
    Reviewed-by: Jakob Linke <jgruber@chromium.org>
    Commit-Queue: Patrick Thier <pthier@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#102838}

Refs: v8/v8@72b0e27
Fixes: #60030
PR-URL: #60732
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
@targos
Copy link
Member

targos commented Dec 7, 2025

Landed in 0bf45a8...c361a62

@targos targos closed this Dec 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. v8 engine Issues and PRs related to the V8 dependency. v24.x Issues that can be reproduced on v24.x or PRs targeting the v24.x-staging branch.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants