lib,src: add source map support for global eval #43428

legendecas · 2022-06-14T17:04:03Z

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

lib/internal/source_map/source_map_cache.js

nodejs-github-bot · 2022-06-16T16:40:16Z

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

bcoe · 2022-06-20T19:47:31Z

@legendecas awesome, will work on reviewing soon.

CC: @nodejs/cpp-reviewers

bcoe · 2022-07-01T15:51:52Z

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

bcoe

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

lib/internal/source_map/prepare_stack_trace.js

src/node_errors.cc

bcoe · 2022-07-05T17:29:19Z

src/node_errors.cc

+  bool codegen_allowed =
+      allow_code_gen->IsUndefined() || allow_code_gen->IsTrue();
+  return {
+      codegen_allowed,


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

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.

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?

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

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.

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?

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.

bcoe · 2022-07-05T17:30:38Z

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

legendecas · 2022-07-06T16:42:05Z

@bcoe thank you for the review. updated :)

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

nodejs-github-bot · 2022-07-09T09:41:23Z

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

nodejs-github-bot · 2022-07-09T12:27:52Z

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

nodejs-github-bot · 2022-07-09T15:55:35Z

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

nodejs-github-bot · 2022-07-09T18:12:02Z

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

nodejs-github-bot · 2022-07-09T20:10:57Z

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

nodejs-github-bot · 2022-07-10T00:23:28Z

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

nodejs-github-bot · 2022-07-10T09:03:26Z

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

nodejs-github-bot · 2022-07-10T12:28:15Z

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

nodejs-github-bot · 2022-07-11T02:24:54Z

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

nodejs-github-bot · 2022-07-11T08:14:54Z

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

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 · 2022-07-11T13:44:43Z

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

nodejs/node#43428

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

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

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

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. 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 Jun 14, 2022

legendecas marked this pull request as ready for review June 14, 2022 17:06

legendecas added the source maps Issues and PRs related to source map support. label Jun 14, 2022

legendecas requested a review from bcoe June 14, 2022 17:41

aduh95 reviewed Jun 15, 2022

View reviewed changes

lib/internal/source_map/source_map_cache.js Outdated Show resolved Hide resolved

lib/internal/source_map/source_map_cache.js Outdated Show resolved Hide resolved

legendecas force-pushed the issue-43047 branch from 62d219c to 3a32d20 Compare June 16, 2022 16:26

legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 16, 2022

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 16, 2022

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 self-assigned this Jun 20, 2022

bcoe approved these changes Jul 5, 2022

View reviewed changes

legendecas force-pushed the issue-43047 branch from 3a32d20 to 38da197 Compare July 6, 2022 16:41

legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 6, 2022

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 6, 2022

legendecas force-pushed the issue-43047 branch 3 times, most recently from 5f49aaa to 9c28953 Compare July 7, 2022 17:36

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 force-pushed the issue-43047 branch from 9c28953 to 775dcb0 Compare July 7, 2022 17:44

legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 8, 2022

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 8, 2022

bcoe approved these changes Jul 9, 2022

View reviewed changes

github-actions bot mentioned this pull request Jul 10, 2022

CI Reliability 2022-07-10 nodejs/reliability#320

Open

20 tasks

legendecas closed this Jul 11, 2022

legendecas deleted the issue-43047 branch July 11, 2022 13:44

legendecas mentioned this pull request Jul 11, 2022

Source maps not supported in eval and new Function #43047

Closed

targos mentioned this pull request Jul 12, 2022

v18.6.0 release proposal #43789

Merged

F3n67u mentioned this pull request Jul 12, 2022

Investigate flaky pummel/test-http-upload-timeout #43800

Closed

github-actions bot mentioned this pull request Jul 13, 2022

CI Reliability 2022-07-13 nodejs/reliability#321

Open

23 tasks

bcoe mentioned this pull request Jul 13, 2022

--enable-source-maps is unnecessarily slow with large source files #41541

Closed

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

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

lib,src: add source map support for global eval

1476ff6

nodejs/node#43428

sapphi-red mentioned this pull request Jan 22, 2023

fix(ssr): load sourcemaps alongside modules vitejs/vite#11780

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib,src: add source map support for global eval #43428

lib,src: add source map support for global eval #43428

legendecas commented Jun 14, 2022

nodejs-github-bot commented Jun 16, 2022

bcoe commented Jun 20, 2022

bcoe commented Jul 1, 2022

bcoe left a comment

bcoe Jul 5, 2022

legendecas Jul 6, 2022

bcoe Jul 6, 2022

legendecas Jul 7, 2022 •

edited

Loading

bcoe Jul 9, 2022

legendecas Jul 9, 2022

bcoe Jul 13, 2022

bcoe commented Jul 5, 2022

legendecas commented Jul 6, 2022

nodejs-github-bot commented Jul 9, 2022

nodejs-github-bot commented Jul 9, 2022

nodejs-github-bot commented Jul 9, 2022

nodejs-github-bot commented Jul 9, 2022

nodejs-github-bot commented Jul 9, 2022

nodejs-github-bot commented Jul 10, 2022

nodejs-github-bot commented Jul 10, 2022

nodejs-github-bot commented Jul 10, 2022

nodejs-github-bot commented Jul 11, 2022

nodejs-github-bot commented Jul 11, 2022

legendecas commented Jul 11, 2022

lib,src: add source map support for global eval #43428

lib,src: add source map support for global eval #43428

Conversation

legendecas commented Jun 14, 2022

nodejs-github-bot commented Jun 16, 2022

bcoe commented Jun 20, 2022

bcoe commented Jul 1, 2022

bcoe left a comment

Choose a reason for hiding this comment

bcoe Jul 5, 2022

Choose a reason for hiding this comment

legendecas Jul 6, 2022

Choose a reason for hiding this comment

bcoe Jul 6, 2022

Choose a reason for hiding this comment

legendecas Jul 7, 2022 • edited Loading

Choose a reason for hiding this comment

bcoe Jul 9, 2022

Choose a reason for hiding this comment

legendecas Jul 9, 2022

Choose a reason for hiding this comment

bcoe Jul 13, 2022

Choose a reason for hiding this comment

bcoe commented Jul 5, 2022

legendecas commented Jul 6, 2022

nodejs-github-bot commented Jul 9, 2022

nodejs-github-bot commented Jul 9, 2022

nodejs-github-bot commented Jul 9, 2022

nodejs-github-bot commented Jul 9, 2022

nodejs-github-bot commented Jul 9, 2022

nodejs-github-bot commented Jul 10, 2022

nodejs-github-bot commented Jul 10, 2022

nodejs-github-bot commented Jul 10, 2022

nodejs-github-bot commented Jul 11, 2022

nodejs-github-bot commented Jul 11, 2022

legendecas commented Jul 11, 2022

legendecas Jul 7, 2022 •

edited

Loading