-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
Use performance.now when possible, re-disable unconditional perf marking on all Node versions #57875
Conversation
@typescript-bot perf test this faster |
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
Many years ago I encountered similar behavior where the time reported by each core on the CPU was slightly different (this was C# code and the behavior didn't repro when I set the thread processor affinity). Unclear if this was a hardware bug or just not an invariant under Windows. |
Yeah, I'm pretty sure that |
Going to rerun this because that result does not seem right at all @typescript-bot perf test this faster |
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
Okay this is definitely slower for some reason. |
I really have no clue what's going on here; I've tried to compare this and there's just no reason for this to be slower. @typescript-bot perf test this faster |
Spectre mitigation, perhaps? |
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
@typescript-bot perf test this faster |
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
I understand what happened; PerformanceObserver is only available as a global on Node 20+. This meant that Node 18 was not getting performance hooks at all as the old code was all or nothing. Then I removed that check, which then re-enabled hooks on Node 18, which is what the faster benchmark uses. Now the order is swapped and so it's no longer enabled in Node at all. Probably this means Node 20 is faster now too. Will update the description and comments and stuff to reflect that. @typescript-bot perf test this full |
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
startupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
Wow, literally 5% faster vscode build time |
@typescript-bot perf test bun |
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
startupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
@@ -116,6 +98,4 @@ export function tryGetNativePerformanceHooks() { | |||
* | |||
* @internal | |||
*/ | |||
export const timestamp = nativePerformance ? () => nativePerformance.now() : | |||
Date.now ? Date.now : | |||
() => +(new Date()); |
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.
While here, removing this branch. Date.now is most definitely defined.
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.
It wasn't guaranteed to be when we targeted ES3, but since we've moved up our minimum target to run tsc this makes sense.
export const timestamp = nativePerformance ? () => nativePerformance.now() : | ||
Date.now ? Date.now : | ||
() => +(new Date()); | ||
export const timestamp = nativePerformanceTime ? () => nativePerformanceTime.now() : Date.now; |
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.
FWIW this must be a closure because Node 19+ require this
to be correct when calling, so we can't just rip off a method.
And just to go back to the original Date.now thing, MDN says:
So, this that probably explains things. Thank you random chance for finding this perf win. |
Now that we're testing different OSs, Node 14 on Windows spuriously fails, like:
https://github.com/microsoft/TypeScript/actions/runs/8367516104/job/22910040628?pr=57870#step:6:1
Somehow, time went backwards???
My guess is that after #50267, native performance hooks only work in node 16.7+ since we wholesale disable native performance support when some parts of it are missing. This also disables the use of
performance.now
, meaning Node 14 is falling back toDate.now()
fortimestamp()
even thoughperformance.now()
would have worked. Now, whyDate.now()
would ever go backwards a millisecond, I have no idea, but the flag isn't happening anywhere other than Node 14, and 14 -> 16 is wheretimestamp()
differs too. Surely not a coincidence...This PR restructures things such that we can still use native performance time functions when available, but still disable the other perf stuff when the mark/measure support is incomplete.
Also, while here, don't check for
PerformanceObserver
, which we haven't used since #42586.Funnily, this has exposed the fact that we have been accidentally enabling native perf marking on Node 20+, because it implements all of the global perf hooks we need and so we "think" it's a browser.
This PR started out as slow because I removed the check for
PerformanceObserver
, which then made Node 18 also satisfy the checks via globals, so we slowed down and that pointed out the problem.