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

v6.x: deps: backport dfb8d33 from V8 upstream #11483

Closed
wants to merge 2 commits into from
Closed

v6.x: deps: backport dfb8d33 from V8 upstream #11483

wants to merge 2 commits into from

Conversation

targos
Copy link
Member

@targos targos commented Feb 21, 2017 •
edited
Loading

Fixes: #11480

I added a regression test in a separate commit that can be squashed while landing.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

v8

@nodejs-github-bot nodejs-github-bot added v6.x v8 engine Issues and PRs related to the V8 dependency. labels Feb 21, 2017
@targos targos mentioned this pull request Feb 21, 2017
Closed
@targos
Copy link
Member Author

targos commented Feb 21, 2017

@nodejs/lts @nodejs/v8

richardlau
richardlau reviewed Feb 22, 2017
View reviewed changes
BUILDING.md Outdated
@@ -121,7 +121,7 @@ Prerequisites:
To run the tests:

```console
> .\vcbuild test
> .\vcbuild nosign test
Copy link
Member

Choose a reason for hiding this comment

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

BUILDING.md changes shouldn't be here.

richardlau
richardlau reviewed Feb 22, 2017
View reviewed changes
CONTRIBUTING.md Outdated
@@ -167,19 +167,19 @@ $ ./configure && make -j4 test
Windows:

```text
> vcbuild test
.\vcbuild nosign test
Copy link
Member

Choose a reason for hiding this comment

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

CONTRIBUTING.md changes shouldn't be here.

@richardlau
Copy link
Member

@targos I think this is including some unrelated commits.

@targos targos force-pushed the backport-destructuring-fix branch from 655b03b to 304997e Compare February 22, 2017 08:21
@targos
Copy link
Member Author

targos commented Feb 22, 2017

CI: https://ci.nodejs.org/job/node-test-pull-request/6551/
V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/582/

@targos
Copy link
Member Author

targos commented Feb 28, 2017

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/614/

@MylesBorins
Copy link
Contributor

@targos would you be able to update the commit message of the regression test to follow our guidelines

targos added 2 commits March 3, 2017 13:56
@targos
deps: backport dfb8d33 from V8 upstream
b858d69 
Original commit message:
    Reduce the memory footprint of expression classifiers

    This patch attempts to reduce the (stack) memory footprint of
    expression classifiers.  Instead of keeping space in each
    classifier for all possible error messages that will
    (potentially) be reported, if an expression turns out to be
    a pattern or a non-pattern, such error messages are placed in
    a list shared by the FunctionState and each classifier keeps a
    couple of indices in this list.  This requires that classifiers
    are used strictly in a stack-based fashion, which is also in line
    with my previous patch for revisiting non-pattern rewriting.

    R=adamk@chromium.org
    BUG=chromium:528697

    Review-Url: https://codereview.chromium.org/1708193003
    Cr-Commit-Position: refs/heads/master@{nodejs#36897}

Fixes: nodejs#11480
@targos
test: add regression test for V8 parse error
6b34806 
Refs: nodejs#11480
@targos targos force-pushed the backport-destructuring-fix branch from 304997e to 6b34806 Compare March 3, 2017 12:59
@targos
Copy link
Member Author

targos commented Mar 3, 2017

@MylesBorins done

MylesBorins pushed a commit that referenced this pull request Mar 8, 2017
@targos @MylesBorins
deps: backport dfb8d33 from V8 upstream
b198918 
Original commit message:
    Reduce the memory footprint of expression classifiers

    This patch attempts to reduce the (stack) memory footprint of
    expression classifiers.  Instead of keeping space in each
    classifier for all possible error messages that will
    (potentially) be reported, if an expression turns out to be
    a pattern or a non-pattern, such error messages are placed in
    a list shared by the FunctionState and each classifier keeps a
    couple of indices in this list.  This requires that classifiers
    are used strictly in a stack-based fashion, which is also in line
    with my previous patch for revisiting non-pattern rewriting.

    R=adamk@chromium.org
    BUG=chromium:528697

    Review-Url: https://codereview.chromium.org/1708193003
    Cr-Commit-Position: refs/heads/master@{#36897}

Fixes: #11480
PR-URL: #11483
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 8, 2017
@targos @MylesBorins
test: add regression test for V8 parse error
b3f32f9 
Refs: #11480
PR-URL: #11483
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
@MylesBorins
Copy link
Contributor

landed in 1c36e6d...b3f32f9

@MylesBorins MylesBorins closed this Mar 8, 2017
MylesBorins pushed a commit that referenced this pull request Mar 8, 2017
@targos @MylesBorins
deps: backport dfb8d33 from V8 upstream
d0e934f 
Original commit message:
    Reduce the memory footprint of expression classifiers

    This patch attempts to reduce the (stack) memory footprint of
    expression classifiers.  Instead of keeping space in each
    classifier for all possible error messages that will
    (potentially) be reported, if an expression turns out to be
    a pattern or a non-pattern, such error messages are placed in
    a list shared by the FunctionState and each classifier keeps a
    couple of indices in this list.  This requires that classifiers
    are used strictly in a stack-based fashion, which is also in line
    with my previous patch for revisiting non-pattern rewriting.

    R=adamk@chromium.org
    BUG=chromium:528697

    Review-Url: https://codereview.chromium.org/1708193003
    Cr-Commit-Position: refs/heads/master@{#36897}

Fixes: #11480
PR-URL: #11483
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 8, 2017
@targos @MylesBorins
test: add regression test for V8 parse error
e71fc70 
Refs: #11480
PR-URL: #11483
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
@targos @MylesBorins
deps: backport dfb8d33 from V8 upstream
3ab070d 
Original commit message:
    Reduce the memory footprint of expression classifiers

    This patch attempts to reduce the (stack) memory footprint of
    expression classifiers.  Instead of keeping space in each
    classifier for all possible error messages that will
    (potentially) be reported, if an expression turns out to be
    a pattern or a non-pattern, such error messages are placed in
    a list shared by the FunctionState and each classifier keeps a
    couple of indices in this list.  This requires that classifiers
    are used strictly in a stack-based fashion, which is also in line
    with my previous patch for revisiting non-pattern rewriting.

    R=adamk@chromium.org
    BUG=chromium:528697

    Review-Url: https://codereview.chromium.org/1708193003
    Cr-Commit-Position: refs/heads/master@{#36897}

Fixes: #11480
PR-URL: #11483
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
@targos @MylesBorins
test: add regression test for V8 parse error
5365501 
Refs: #11480
PR-URL: #11483
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
Merged
@mscdex mscdex mentioned this pull request Mar 21, 2017
Closed
@targos targos deleted the backport-destructuring-fix branch June 1, 2017 08:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richardlau richardlau richardlau left review comments

@MylesBorins MylesBorins MylesBorins approved these changes

@jasnell jasnell jasnell approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

Assignees
No one assigned
Labels
v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@targos @richardlau @MylesBorins @bnoordhuis @jasnell @nodejs-github-bot