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

lib: suppress source map lookup exceptions #56299

Merged
merged 2 commits into from
Dec 20, 2024
Merged

lib: suppress source map lookup exceptions #56299

merged 2 commits into from
Dec 20, 2024

Conversation

legendecas
Copy link
Member

@legendecas legendecas commented Dec 17, 2024 •
edited
Loading

lib: skip parsing invalid source maps

When the source map data are invalid json strings, skip construct
SourceMap on it. Additionally, suppress exceptions on source map lookups
and fix test runners crash on invalid source maps.

lib: optimize prepareStackTrace on builtin frames

Only invalidates source map lookup cache when a new source map is found.
This improves when user codes interleave with builtin functions, like
array.map.

                                                      confidence improvement accuracy (*)   (**)  (***)
es/error-stack.js n=100000 method='sourcemap'                ***     78.04 %       ±1.27% ±1.70% ±2.24%
es/error-stack.js n=100000 method='without-sourcemap'                 0.27 %       ±1.09% ±1.45% ±1.89%

Refs: #56296

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/performance
  • @nodejs/test_runner

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Dec 17, 2024
@legendecas legendecas added source maps Issues and PRs related to source map support. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. labels Dec 17, 2024
@legendecas legendecas mentioned this pull request Dec 18, 2024
Closed
@legendecas legendecas added the test_runner Issues and PRs related to the test runner subsystem. label Dec 18, 2024
@codecov Codecov
Copy link

codecov bot commented Dec 18, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.55%. Comparing base (990497c) to head (28557ef).
Report is 2 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main   #56299   +/-   ##
=======================================
  Coverage   88.55%   88.55%           
=======================================
  Files         657      657           
  Lines      190295   190301    +6     
  Branches    36542    36544    +2     
=======================================
+ Hits       168511   168529   +18     
- Misses      14969    14973    +4     
+ Partials     6815     6799   -16
Files with missing lines Coverage Δ
lib/internal/source_map/prepare_stack_trace.js 95.56% <100.00%> (-1.13%) ⬇️

... and 38 files with indirect coverage changes

mcollina
mcollina approved these changes Dec 18, 2024
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

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

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

aduh95 pushed a commit to legendecas/node that referenced this pull request Dec 20, 2024
@legendecas @aduh95
lib: suppress source map lookup exceptions
4f85458 
When the source map data are invalid json strings, skip construct
SourceMap on it. Additionally, suppress exceptions on source map lookups
and fix test runners crash on invalid source maps.

PR-URL: nodejs#56299
Refs: nodejs#56296
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Xuguang Mei <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Pietro Marchini <[email protected]>
aduh95 pushed a commit to legendecas/node that referenced this pull request Dec 20, 2024
@legendecas @aduh95
lib: optimize prepareStackTrace on builtin frames
7e3b0df 
Only invalidates source map lookup cache when a new source map is found.
This improves when user codes interleave with builtin functions, like
`array.map`.

PR-URL: nodejs#56299
Refs: nodejs#56296
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Xuguang Mei <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Pietro Marchini <[email protected]>
@aduh95 aduh95 force-pushed the source-maps/path-url branch from eacdbb6 to 7e3b0df Compare December 20, 2024 00:12
@legendecas @aduh95
lib: suppress source map lookup exceptions
a33ec12 
When the source map data are invalid json strings, skip construct
`SourceMap` on it. Additionally, suppress exceptions on source map
lookups and fix test runners crash on invalid source maps.

PR-URL: nodejs#56299
Refs: nodejs#56296
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Xuguang Mei <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Pietro Marchini <[email protected]>
@legendecas @aduh95
lib: optimize prepareStackTrace on builtin frames
28557ef 
Only invalidates source map lookup cache when a new source map is found.
This improves when user codes interleave with builtin functions, like
`array.map`.

PR-URL: nodejs#56299
Refs: nodejs#56296
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Xuguang Mei <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Pietro Marchini <[email protected]>
@aduh95 aduh95 force-pushed the source-maps/path-url branch from 7e3b0df to 28557ef Compare December 20, 2024 00:13
@aduh95
Copy link
Contributor

aduh95 commented Dec 20, 2024

Landed in 990497c...28557ef

@aduh95 aduh95 merged commit 28557ef into nodejs:main Dec 20, 2024
20 checks passed
@legendecas legendecas deleted the source-maps/path-url branch December 20, 2024 01:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@atlowChemi atlowChemi atlowChemi approved these changes

@pmarchini pmarchini pmarchini approved these changes

@meixg meixg meixg approved these changes

@cjihrig cjihrig cjihrig approved these changes

@anonrig anonrig anonrig approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. needs-ci PRs that need a full CI run. source maps Issues and PRs related to source map support. test_runner Issues and PRs related to the test runner subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@legendecas @nodejs-github-bot @aduh95 @mcollina @anonrig @cjihrig @meixg @pmarchini @atlowChemi