Skip to content

Add raf based measurement - #173

Merged
rniwa merged 1 commit into
mainfrom
add-raf-based-measurement
May 24, 2023
Merged

Add raf based measurement#173
rniwa merged 1 commit into
mainfrom
add-raf-based-measurement

Conversation

@rniwa

@rniwa rniwa commented May 20, 2023

Copy link
Copy Markdown
Member

This PR adds a new query string parameter requestAnimationFrame, which takes the value of either "timer" or "raf". "timer" is the existing and default measurement method. When it's "raf", we use two consecutive requestAnimationFrame first of which runs the sync step and second of which schedules a 0s timer to signify the end of async step.

@rniwa
rniwa force-pushed the add-raf-based-measurement branch 2 times, most recently from 11ab3c4 to 142589c Compare May 20, 2023 03:30
Comment thread resources/benchmark-runner.mjs Outdated
let syncTime;
let asyncStartTime;
let asyncTime;
const invokerClass = params.measurementMethod === 'raf' ? RAFTestInvoker : TimerTestInvoker;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nice!

@camillobruni

Copy link
Copy Markdown
Contributor

Modulo function-names / bind comment. Looking good!
Thanks for pulling this apart :)

Comment thread resources/benchmark-runner.mjs Outdated
let asyncStartTime;
let asyncTime;
const invokerClass = params.measurementMethod === 'raf' ? RAFTestInvoker : TimerTestInvoker;
const invoker = new invokerClass(() => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

As on the other PR, maybe named-methods + bind makes this more readable?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

What do you mean by that? Could you show me an example?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Along these lines (implying moving the *time values onto the class):

   const invoker = new invokerClass(this.measureSync.bind(this), this.measureAsync.bind(this), this._report.bind(this));
    ...
  }
  measureSync(...) {
   ...
  }
  measureAsync(...) {
    ...
  }
  ...

Maybe easier: using named arrow functions would be an improvement:

   ...
   const syncStart = () => {...};
   const measureAsync = () => {...};
   const report = () => {...};
   const invoker = new invokerClass(measureSync, measureAsync, report);
   

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

.bind() makes it harder to read ;)
But that latter case looks better to me.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done that.

return this._recordTestResults(suite, test, syncTime, asyncTime);
});

return invoker.start();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe await invoker.start();

@smaug---- smaug---- left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

r+ for the actual benchmark-runner. (I'm less familiar with test tests/* code).
Perhaps assign the callbacks to variables and pass those as params, that might improve the readability a bit.

@rniwa
rniwa force-pushed the add-raf-based-measurement branch from 142589c to 74bbaf6 Compare May 24, 2023 06:38
This PR adds a new query string parameter requestAnimationFrame, which takes
the value of either "timer" or "raf". "timer" is the existing and default
measurement method. When it's "raf", we use two consecutive requestAnimationFrame
first of which runs the sync step and second of which schedules a 0s timer to
signify the end of async step.
@rniwa
rniwa force-pushed the add-raf-based-measurement branch from 74bbaf6 to d2750a4 Compare May 24, 2023 18:49
@rniwa
rniwa merged commit a876f0e into main May 24, 2023
@rniwa
rniwa deleted the add-raf-based-measurement branch May 24, 2023 18:50
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.

4 participants