GV

GeezerVison

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

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — lib,src: add source map support for global eval by legendecas · Pull Request #43428 · nodejs/node

Conversation

@legendecas

@legendecas legendecas commented Jun 14, 2022

Copy link
Copy Markdown
Member

Add source map support for global eval.

Dynamic sources with FunctionConstructor are not supported yet as
V8 prepends lines to the sources which makes the stack line number
incorrect.

Refs: #43047

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
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. labels Jun 14, 2022
@legendecas
legendecas marked this pull request as ready for review June 14, 2022 17:06
@legendecas legendecas added the source maps Issues and PRs related to source map support. label Jun 14, 2022
@legendecas
legendecas requested a review from bcoe June 14, 2022 17:41
aduh95
aduh95 reviewed Jun 15, 2022
View reviewed changes
Comment thread lib/internal/source_map/source_map_cache.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread lib/internal/source_map/source_map_cache.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
@legendecas
legendecas force-pushed the issue-43047 branch from 62d219c to 3a32d20 Compare June 16, 2022 16:26
@legendecas legendecas 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 Jun 16, 2022
@github-actions 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 Jun 16, 2022
@nodejs-github-bot

nodejs-github-bot commented Jun 16, 2022

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/44626/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
This was referenced Jun 17, 2022
CI Reliability 2022-06-17 nodejs/reliability#303
Open
CI Reliability 2022-06-18 nodejs/reliability#304
Open
CI Reliability 2022-06-19 nodejs/reliability#305
Open
@bcoe bcoe self-assigned this Jun 20, 2022
@bcoe

bcoe commented Jun 20, 2022

Copy link
Copy Markdown
Contributor

@legendecas awesome, will work on reviewing soon.

CC: @nodejs/cpp-reviewers
Kifzy reacted with thumbs up emoji legendecas reacted with rocket emoji
All reactions
  • 👍 1 reaction
  • 🚀 1 reaction

Sorry, something went wrong.

Uh oh!

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

bcoe commented Jul 1, 2022

Copy link
Copy Markdown
Contributor

@legendecas I have a 4 day weekend, hoping to get through my backlog of reviews.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bcoe
bcoe approved these changes Jul 5, 2022
View reviewed changes

@bcoe bcoe left a comment

Copy link
Copy Markdown
Contributor

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.

I love this, much simpler implementation than I would have guessed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread lib/internal/source_map/prepare_stack_trace.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/node_errors.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/node_errors.cc Outdated

@bcoe bcoe Jul 5, 2022

Copy link
Copy Markdown
Contributor

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.

If no callback is provided, is codegen_allowed just always truthy?

Sorry, something went wrong.

Uh oh!

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

@legendecas legendecas Jul 6, 2022

Copy link
Copy Markdown
Member Author

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.

If the value set in v8::Context::AllowCodeGenerationFromStrings is true, no host-callbacks are called on compiling dynamic sources.

The callback is called only when AllowCodeGenerationFromStrings(false) is set on the context. Thus if the callback is not provided when AllowCodeGenerationFromStrings is set to false, no dynamic sources are allowed.

Sorry, something went wrong.

Uh oh!

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

@bcoe bcoe Jul 6, 2022

Copy link
Copy Markdown
Contributor

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.

Prior to this, did v8::Context::AllowCodeGenerationFromStrings default to true?

Any concerns about performance hits we might observe now that we're calling a handler? Perhaps it would be worth adding a benchmark?

Sorry, something went wrong.

Uh oh!

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

@legendecas legendecas Jul 7, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member Author

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.

@bcoe thank you for your suggestion. I added a new fast-path on if sourcemap is enabled in ModifyCodeGenerationFromStrings and ran a benchmark against the main branch. Now the difference when sourcemap is not enabled can be slight if the dynamic code is not trivial.

However, with sourcemap enabled, the difference can be significant. But I suppose this is acceptable since sourcemap comes with costs.

                                                confidence improvement accuracy (*)   (**)  (***)
es/eval.js n=1000000 method='without-sourcemap'        ***     -1.54 %       ±0.36% ±0.49% ±0.63%
es/eval.js n=1000000 method='sourcemap'        ***    -31.90 %       ±1.41% ±1.89% ±2.49%

Sorry, something went wrong.

Uh oh!

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

@bcoe bcoe Jul 9, 2022

Copy link
Copy Markdown
Contributor

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.

thanks for adding the benchmark 👌

I'm curious, how much more does it slow down if you have the noop closure, without the fast path?

However, with sourcemap enabled, the difference can be significant. But I suppose this is acceptable since sourcemap comes with costs.

I think this is to be expected. Unrelated to this PR, I'm becoming convinced we should stop performing the logic that loads the original source from disk for providing visual context, as I believe this is one of the major slow downs.

Sorry, something went wrong.

Uh oh!

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

@legendecas legendecas Jul 9, 2022

Copy link
Copy Markdown
Member Author

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.

The slowdown can be up to ~25% without the fast path with this micro benchmark.

I think this is to be expected. Unrelated to this PR, I'm becoming convinced we should stop performing the logic that loads the original source from disk for providing visual context, as I believe this is one of the major slow downs.

Yeah, when sourcemap support is not enabled, the visual context is not prepended to the value of the stack property. It's just being prepended when the error is being printed by the runtime like fatal exceptions. While accessing stack property is one of the most commonly used paths, improving the performance of it is always a good motivation. Are you planning to work on it?

Sorry, something went wrong.

Uh oh!

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

@bcoe bcoe Jul 13, 2022

Copy link
Copy Markdown
Contributor

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.

Are you planning to work on it?

I don't think I'll have cycles to work on this in the short term, I'll CC you on the pertinent issue.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bcoe

bcoe commented Jul 5, 2022

Copy link
Copy Markdown
Contributor

@legendecas rebase and let's see about landing this?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@legendecas
legendecas force-pushed the issue-43047 branch from 3a32d20 to 38da197 Compare July 6, 2022 16:41
@legendecas legendecas 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, 2022
@legendecas

legendecas commented Jul 6, 2022

Copy link
Copy Markdown
Member Author

@bcoe thank you for the review. updated :)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions 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, 2022
@legendecas
legendecas force-pushed the issue-43047 branch 3 times, most recently from 5f49aaa to 9c28953 Compare July 7, 2022 17:36
@legendecas
lib,src: add source map support for global eval
775dcb0 
Dynamic sources with FunctionConstructor is not supported yet as
V8 prepends lines to the sources which makes the stack line number
incorrect.
@legendecas
legendecas force-pushed the issue-43047 branch from 9c28953 to 775dcb0 Compare July 7, 2022 17:44
@legendecas legendecas 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 8, 2022
@github-actions 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 8, 2022
@nodejs-github-bot

nodejs-github-bot commented Jul 9, 2022

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/45204/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Jul 9, 2022

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/45214/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bcoe
bcoe approved these changes Jul 9, 2022
View reviewed changes
@nodejs-github-bot

nodejs-github-bot commented Jul 9, 2022

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/45224/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Jul 9, 2022

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/45229/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Jul 9, 2022

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/45236/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot mentioned this pull request Jul 10, 2022
CI Reliability 2022-07-10 nodejs/reliability#320
Open
20 tasks
@nodejs-github-bot

nodejs-github-bot commented Jul 10, 2022

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/45243/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Jul 10, 2022

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/45259/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Jul 10, 2022

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/45267/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Jul 11, 2022

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/45283/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Jul 11, 2022

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/45291/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
legendecas added a commit that referenced this pull request Jul 11, 2022
@legendecas
lib,src: add source map support for global eval
02eb10b 
Dynamic sources with FunctionConstructor is not supported yet as
V8 prepends lines to the sources which makes the stack line number
incorrect.

PR-URL: #43428
Refs: #43047
Reviewed-By: Ben Coe <[email protected]>
@legendecas

legendecas commented Jul 11, 2022

Copy link
Copy Markdown
Member Author

Landed in 02eb10b
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@legendecas legendecas closed this Jul 11, 2022
@legendecas
legendecas deleted the issue-43047 branch July 11, 2022 13:44
@legendecas legendecas mentioned this pull request Jul 11, 2022
Source maps not supported in eval and new Function #43047
Closed
targos pushed a commit that referenced this pull request Jul 12, 2022
@legendecas @targos
lib,src: add source map support for global eval
61e6d78 
Dynamic sources with FunctionConstructor is not supported yet as
V8 prepends lines to the sources which makes the stack line number
incorrect.

PR-URL: #43428
Refs: #43047
Reviewed-By: Ben Coe <[email protected]>
@targos targos mentioned this pull request Jul 12, 2022
v18.6.0 release proposal #43789
Merged
@F3n67u F3n67u mentioned this pull request Jul 12, 2022
Investigate flaky pummel/test-http-upload-timeout #43800
Closed
@github-actions github-actions Bot mentioned this pull request Jul 13, 2022
CI Reliability 2022-07-13 nodejs/reliability#321
Open
23 tasks
@bcoe bcoe mentioned this pull request Jul 13, 2022
--enable-source-maps is unnecessarily slow with large source files #41541
Closed
targos pushed a commit that referenced this pull request Jul 31, 2022
@legendecas @targos
lib,src: add source map support for global eval
abc134c 
Dynamic sources with FunctionConstructor is not supported yet as
V8 prepends lines to the sources which makes the stack line number
incorrect.

PR-URL: #43428
Refs: #43047
Reviewed-By: Ben Coe <[email protected]>
@targos targos mentioned this pull request Aug 3, 2022
v16.17.0 release proposal #44098
Merged
codebytere added a commit to electron/electron that referenced this pull request Aug 24, 2022
@codebytere
lib,src: add source map support for global eval
c461028 
nodejs/node#43428
codebytere added a commit to electron/electron that referenced this pull request Aug 25, 2022
@codebytere
lib,src: add source map support for global eval
1476ff6 
nodejs/node#43428
jkleinsc pushed a commit to electron/electron that referenced this pull request Aug 29, 2022
@electron-roller @codebytere
chore: bump node to v16.17.0 (main) (#35350)
d0e220c 
* chore: bump node in DEPS to v16.17.0

* chore: fixup asar patch

* lib: use null-prototype objects for property descriptors

nodejs/node#43270

* src: make SecureContext fields private

nodejs/node#43173

* crypto: remove Node.js-specific webcrypto extensions

nodejs/node#43310

* test: refactor to top-level await

nodejs/node#43500

* deps: cherry-pick two libuv fixes

nodejs/node#43950

* src: slim down env-inl.h

nodejs/node#43745

* util: add AggregateError.prototype.errors to inspect output

nodejs/node#43646

* esm: improve performance & tidy tests

nodejs/node#43784

* src: NodeArrayBufferAllocator delegates to v8's allocator

nodejs/node#43594

* chore: update patch indices

* chore: update filenames

* src: refactor IsSupportedAuthenticatedMode

nodejs/node#42368

* src: add --openssl-legacy-provider option

nodejs/node#40478

* lib,src: add source map support for global eval

nodejs/node#43428

* trace_events: trace net connect event

nodejs/node#43903

* deps: update ICU to 71.1

nodejs/node#42655

This fails the test because it's missing https://chromium-review.googlesource.com/c/chromium/deps/icu/+/3841093

* lib: give names to promisified exists() and question()

nodejs/node#43218

* crypto: add CFRG curves to Web Crypto API

nodejs/node#42507

* src: fix memory leak for v8.serialize

nodejs/node#42695

This test does not work for Electron as they do not use V8's
ArrayBufferAllocator.

* buffer: fix atob input validation

nodejs/node#42539

* src: fix ssize_t error from nghttp2.h

nodejs/node#44393

Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com>
Co-authored-by: Shelley Vohr <[email protected]>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
@legendecas @guangwong
lib,src: add source map support for global eval
710c66d 
Dynamic sources with FunctionConstructor is not supported yet as
V8 prepends lines to the sources which makes the stack line number
incorrect.

PR-URL: nodejs/node#43428
Refs: nodejs/node#43047
Reviewed-By: Ben Coe <[email protected]>
@sapphi-red sapphi-red mentioned this pull request Jan 22, 2023
fix(ssr): load sourcemaps alongside modules vitejs/vite#11780
Merged
6 of 9 tasks
khalwa pushed a commit to solarwindscloud/electron that referenced this pull request Feb 22, 2023
@electron-roller @codebytere @khalwa
chore: bump node to v16.17.0 (main) (electron#35350)
a2ca4b7 
* chore: bump node in DEPS to v16.17.0

* chore: fixup asar patch

* lib: use null-prototype objects for property descriptors

nodejs/node#43270

* src: make SecureContext fields private

nodejs/node#43173

* crypto: remove Node.js-specific webcrypto extensions

nodejs/node#43310

* test: refactor to top-level await

nodejs/node#43500

* deps: cherry-pick two libuv fixes

nodejs/node#43950

* src: slim down env-inl.h

nodejs/node#43745

* util: add AggregateError.prototype.errors to inspect output

nodejs/node#43646

* esm: improve performance & tidy tests

nodejs/node#43784

* src: NodeArrayBufferAllocator delegates to v8's allocator

nodejs/node#43594

* chore: update patch indices

* chore: update filenames

* src: refactor IsSupportedAuthenticatedMode

nodejs/node#42368

* src: add --openssl-legacy-provider option

nodejs/node#40478

* lib,src: add source map support for global eval

nodejs/node#43428

* trace_events: trace net connect event

nodejs/node#43903

* deps: update ICU to 71.1

nodejs/node#42655

This fails the test because it's missing https://chromium-review.googlesource.com/c/chromium/deps/icu/+/3841093

* lib: give names to promisified exists() and question()

nodejs/node#43218

* crypto: add CFRG curves to Web Crypto API

nodejs/node#42507

* src: fix memory leak for v8.serialize

nodejs/node#42695

This test does not work for Electron as they do not use V8's
ArrayBufferAllocator.

* buffer: fix atob input validation

nodejs/node#42539

* src: fix ssize_t error from nghttp2.h

nodejs/node#44393

Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com>
Co-authored-by: Shelley Vohr <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aduh95 aduh95 aduh95 left review comments
+1 more reviewer
@bcoe bcoe bcoe approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

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

Assignees

@bcoe bcoe

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. source maps Issues and PRs related to source map support.

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

@legendecas @nodejs-github-bot @bcoe @aduh95
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.