-
Notifications
You must be signed in to change notification settings - Fork 4.3k
cpu-prof: match Node/Deno Chrome DevTools format #29241
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.
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
Merged
Jarred-Sumner
merged 17 commits into
main
from
farm/33533379/cpuprof-linenumber-matches-node
Apr 14, 2026
+378
−290
Merged
Changes from 3 commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
5cdb657
cpu-prof: match Node/Deno Chrome DevTools format
robobun 0e9c177
[autofix.ci] apply automated fixes
autofix-ci[bot] 57bf345
cpu-prof: carry the def-mapped URL through, tighten positionTicks test
robobun 36ee468
cpu-prof test: assert sum(positionTicks) <= hitCount in sourcemap pat…
robobun 97bfb9e
cpu-prof test: trim top-of-file prose, bump per-test timeout to 30s
robobun 50db0ef
[autofix.ci] apply automated fixes
autofix-ci[bot] e840181
cpu-prof: delete dead stopCPUProfilerAndGetJSON
robobun a1756db
cpu-prof test: gate on isDebug so release CI lanes skip
robobun fab5f13
ci: retrigger after darwin-13-aarch64 runner flake
robobun a9c4af2
cpu-prof test: revert skipIf(!isDebug), let release lanes run
robobun a811a82
cpu-prof test: drop explicit timeout override
robobun 09dfc0c
[autofix.ci] apply automated fixes
autofix-ci[bot] 2bc31ad
cpu-prof: drop sample line when source maps to a different file
robobun 538c046
cpu-prof: normalize callFrame.url to file:// after sourcemap calls
robobun 68bac11
cpu-prof test: rename unused stderr → _stderr, fix stale comment
robobun a60bbbe
cpu-prof: fix misleading sampleURL comment + tighten fib merge key
robobun 8633b6c
ci: retrigger gate after staging webkit caches
robobun File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
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.
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 comment on
positionTicksin theProfileNodestruct cites incorrect sentinel values: it says the defaultIntHashTraitsreserves0and-1, butWTF::HashTraits<int>(the actual key traits) uses0as the empty sentinel andstd::numeric_limits<int>::max()(INT_MAX) as the deleted sentinel — not-1. The code is functionally correct since source line numbers are always small positive integers that can never equal INT_MAX, but the comment misstates the reserved values and names a nonexistent WTF type.Extended reasoning...
The comment at lines 80-81 reads: "the default IntHashTraits (which reserve 0 and -1 as empty/deleted sentinels) are safe here." This has two inaccuracies.
First, "IntHashTraits" is not a WTF type name. The HashMap declaration is
WTF::HashMap<int, int, WTF::IntHash<int>>, whereWTF::IntHash<int>is the hash function and the key traits default toWTF::HashTraits<int>. The comment conflates the hash function name with the traits type name.Second, the sentinel values cited (-1 as deleted) are likely wrong. Modern WebKit WTF defines
HashTraits<int>viaIntegralHashTraits<int>, which sets the empty sentinel toT()(0) viaemptyValueIsZero = true, and the deleted sentinel tostd::numeric_limits<T>::max()(INT_MAX forint). The comment's claim of-1as the deleted sentinel does not match this type.One refuting verifier argued that
GenericHashTraits<T>for signed integer types usesT(-1)as the deleted sentinel, making the comment correct. However, multiple confirming verifiers independently traced the trait hierarchy toIntegralHashTraits<int>with INT_MAX as the deleted value. Regardless of which exact sentinel value is used, the type name "IntHashTraits" is not a real WTF type and will mislead any reader who tries to look it up.The runtime impact is zero:
sampleLineis always a 1-indexed source line number populated only whensourceMappedLineColumn.line > 0, making it a small positive integer that cannot collide with 0, -1, INT_MAX, or INT_MAX-1. The safety invariant holds regardless of which sentinel values WTF actually uses.The risk of the misleading comment is that a future developer might: (a) add a spurious
sampleLine != -1guard believing -1 is a sentinel; (b) rely on thesampleLine > 0guard as sufficient protection against the alleged "0 sentinel" while missing the actual INT_MAX sentinel; or (c) spend time searching for the nonexistent "IntHashTraits" type. The fix is to replace the comment with the correct type name (WTF::HashTraits<int>) and accurate sentinel values, along with the real safety justification: source line numbers are small positive integers, nowhere near INT_MAX.