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
refactor: performance timers for node.js and browser #4340
Merged
Merged
Changes from 10 commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
058e471
refactor: performance timers for node.js and browser
dnalborczyk b23901e
simplify, add return types
dnalborczyk 2640e8f
refactor: replace object with array
dnalborczyk 60a6ddb
refactor: replace fn.apply with spread
dnalborczyk 0bcb7a3
refactor: use map
dnalborczyk b980319
merge master
dnalborczyk 91b12f4
nit
dnalborczyk 08ecfe2
merge master
dnalborczyk fea5f4b
export performance object
dnalborczyk e9fd612
type fixes
dnalborczyk 9b8034b
fix browser perf test, add comment
dnalborczyk c245745
export entire process object
dnalborczyk aab30f4
remove type assertion in rollup.config
dnalborczyk 20630a7
remove comment
dnalborczyk 23a2217
extract interface
dnalborczyk 614c0f6
check for performance object in alternative environments
dnalborczyk 22cc2c5
Merge branch 'master' into perf-timers
lukastaegert File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
export default performance; | ||
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
export default function getMemory(): 0 { | ||
return 0; | ||
} |
This file contains 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
This file contains 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
This file contains 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
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
export { performance as default } from 'perf_hooks'; |
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
import { memoryUsage } from 'process'; | ||
|
||
export default function getMemory(): number { | ||
return memoryUsage().heapUsed; | ||
} |
This file contains 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
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.
Ignore this comment if it's a known issue with this Draft PR.
I think a level of indirection is needed here - via
typeof
orfunction
. Although this would work in a recent browser, referencing the globalperformance
at top level causes the browser bundle and its tests to fail on node <= v14: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.
yeah, ideally the global performance object would be used for both browser and node.js. I was under the impression it was supported as a global in v14, but you are right, it's only available in v16, unless someone would do a backport 😞
if I understood the bundle process correctly, the file used for the node.js build would be this one: https://github.com/rollup/rollup/pull/4340/files#diff-56aca2bfe43f23ab215af1f0c17d5ac2abbed4deb560487408bc588c47c2fc44R1 being replaced with this call:
rollup/rollup.config.ts
Line 160 in da3bb43
originally I wrapped and exported the
now
function only, but I was thinking it's probably better to make the entireperformance
object (for future use) available. I was also thinking to remove the timer Map, and useperformance.measure
and/orperformance observer
instead. but it seems currently it wouldn't be possible to attach any other metadata, e.g. for the value for the memory consumption. (detail
param was added in v16)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.
ah, I see. did you mean this: https://github.com/rollup/rollup/runs/4723627853?check_suite_focus=true#step:7:5615 just noticed. I have to check it out. I suppose those tests are not running in any browser but in node.js 🤔
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.
Yeah, the browser bundle name is a bit of a misnomer. It really means JS platform independent.