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

Resolve pc.now to native function #4162

Merged
merged 1 commit into from
Apr 2, 2022

Conversation

kungfooman
Copy link
Collaborator

I was doing some benchmarking and even now appeared, so I tried to optimize it somewhat:

image

I made this little benchmark:

console.clear();
const n = 1_000_000;
function runDateNow() {
  for (var i=0; i<n; i++) {
    Date.now();
  }
}
const dateNow = Date.now;
function runDateBound() {
  for (var i=0; i<n; i++) {
    dateNow();
  }
}
function pcNow() {
  return window.performance.now();
}
function runNow() {
  for (var i=0; i<n; i++) {
    pcNow();
  }
}
const pcNowBound = performance.now.bind(performance);
function runNowBound() {
  for (var i=0; i<n; i++) {
    pcNowBound();
  }
}
tests = [
  runDateNow,
  runNow,
  runNowBound,
];
results = [];
function testAll() {
  for (const test of tests) {
    const start = performance.now();
    test();
    const end = performance.now();
    results.push([test.name, Math.round(end - start)]);
    console.clear();
    results.sort((a, b) => {
      const ret = a[0].localeCompare(b[0]);
      if (ret === 0) {
        return b[1] - a[1];
      }
      return ret;
    });
    console.table(results);
  }
}
testAll();
testAll();
testAll();
testAll();
testAll();
testAll();

(can be pasted in F12/devtools for quick testing)

Results (number is milliseconds, smaller = faster):

image

TLDR: less code + faster, because its not going through the extra function callback anymore:

image

Previously:

image

I confirm I have read the contributing guidelines and signed the Contributor License Agreement.

@LeXXik
Copy link
Contributor

LeXXik commented Apr 1, 2022

I think performance.now() has a higher precision and is a better fit for measuring time between stamps, than Date.now(). Perhaps there could be 2 versions, one with Date.now() and used in, say, UI buttons, where high precision is not needed. The other would then be used in debug builds for measuring performance times.

@mvaligursky
Copy link
Contributor

This looks fine to me .. tiny improvement per call, but why not. Functionality is the same, it just avoids the look up from the window.

The reason this shows on the profiler is not that the function is slow .. it takes around 0.001ms per call based on your numbers. But browser runs sampling profiler at low frequency to avoid impact of the profiler, and the sample hit the function directly. Next sample was 0.11ms later and hit different function. It does not mean the now took 0.11ms there.

@kungfooman
Copy link
Collaborator Author

I think performance.now() has a higher precision and is a better fit for measuring time between stamps, than Date.now(). Perhaps there could be 2 versions, one with Date.now() and used in, say, UI buttons, where high precision is not needed. The other would then be used in debug builds for measuring performance times.

For my sake both timing functions are fast enough, what I want to focus on is the difference between two kinds of calls:

Current now implementation, going through an extra function and window lookup:

function pcNow() {
  return window.performance.now();
}

And as bound function which is called as native bound function without wrapper function nor window lookup:

const pcNowBound = performance.now.bind(performance);

Functionality is the same, it just avoids the look up from the window.

Yep, in my projects I apply many getters/setters to window and then the window lookup can make more impact than usual:

image

(its relatively 6x faster, but not much overall as you say)

Copy link
Contributor

@mvaligursky mvaligursky left a comment

Choose a reason for hiding this comment

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

looks good to me, I don't see any problems with this.

@willeastcott willeastcott merged commit a10b2dd into playcanvas:main Apr 2, 2022
@kungfooman kungfooman deleted the optimizeNow branch November 11, 2023 10:24
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.

None yet

4 participants