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.

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

feat: add memory-spike plugin #33

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

naugtur
Copy link
Contributor

@naugtur naugtur commented Dec 25, 2024

it took going on holiday again, but here it is

open questions:

  • not sure what the expectation is on report string and which is more important for a plugin's end user - report or result
  • I don't enjoy having to normalize the measurements by dividing them into potentially small numbers and aggregating again' but that's how the benchmarking loop is designed and I understand having a before and after hook for an entire case in a suite may not be feasible
  • is adding a global reference to __recordMemorySpike going to cause trouble in workers?
  • how do you like the test? It's the first plugin with a test

Copy link
Owner

@RafaelGSS RafaelGSS left a comment

Choose a reason for hiding this comment

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

Instead of creating a function inside globalThis, I'd add all the context into the beforeClockTemplate. Also, I wouldn't perform memory translations during the collect process, instead, you can do it textReporter. Just check if bench.plugins includes this new plugin.

#spikeSamples = {};
isSupported() {
try {
new Function(`gc()`);
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
new Function(`gc()`);
new Function(`gc()`)();

I think you should call the function to it to throw, right?

Comment on lines +100 to +103
getReport() {
process._rawDebug('grp',arguments);

}
Copy link
Owner

Choose a reason for hiding this comment

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

This shouldn't be here, right?


### Class: `RecordMemorySpikePlugin`

A plugin to record memory allocation spikes in your benchmark's run. It should help you understand the speed vs memory tradeoffs you're making.
Copy link
Owner

Choose a reason for hiding this comment

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

We should mention folks that are going to use it will need to pass --expose-gc

Copy link
Owner

Choose a reason for hiding this comment

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

I'd also mention a possible performance inconsistency when this plugin is enabled due to GC blocking op.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants