Skip to content

Add performance framework from transforms branch #9536

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

Merged
merged 5 commits into from
Jul 19, 2016

Conversation

weswigham
Copy link
Member

@weswigham weswigham commented Jul 6, 2016

@rbuckton
@mhegazy

I was working on suspiciously similar code for doing extension profiling in #9038 - by splitting the performance tools framework from either PR, we can merge them in more quickly and start using a common framework across all PRs.

Notable differences from the original code in the transforms branch:

  • When the new extendedDiagnostics argument is supplied, all recorded perf buckets are reported under the friendly name they use as their key (unlike before, where they used a codename and had to be explicitly printed). This is very useful since extensions are likely to add extra perf buckets on the fly (and we have no idea what they'll be named, but would still like to report them in the summary).
  • The counter and emit code is presently unused - @rbuckton uses them in profiling and inspecting transformations in his branch, but I don't yet have a need to insert profiling events or replace counters elsewhere in our codebase (and I may use them for extensions, too).
  • The function used to mark internally is now chosen as one of performance.now, Date.now, and () => new Date().getTime() (in that order) - this way a higher resolution/performance marker is used when available, but falls back to a universal option if they are unavailable. (And as @rbuckton explained the other day, it doesn't try to use process.hrtime since that would allocate arrays.)
  • It's in its own file, rather than in core.ts - it's an isolated set of work and is understandable/reusable. IMO, it makes sense for it to be its own unit.

const checkTime = performance.getDuration("Check");
const emitTime = performance.getDuration("Emit");
if (compilerOptions.extendedDiagnostics) {
performance.forEachMeasure((name, duration) => reportTimeStatistic(`${name} time`, duration));
Copy link
Contributor

Choose a reason for hiding this comment

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

With this you'll get different names for statistics than you do below (e.g. "I/O read time" vs. "I/O read").

Copy link
Member Author

@weswigham weswigham Jul 11, 2016

Choose a reason for hiding this comment

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

True. I could just special-case out the builtins while printing and print them via the old path (though I'd honestly prefer using a single code path if renaming them is okay).

@rbuckton
Copy link
Contributor

👍

@weswigham weswigham merged commit 2a26beb into microsoft:master Jul 19, 2016
@weswigham weswigham deleted the performance-refactor branch July 19, 2016 22:10
@mihailik
Copy link
Contributor

mihailik commented Jul 20, 2016

Measurements suggest the +new Date() is better that new Date().getTime()

The benchmark code below, runnable in WSH/CSCRIPT, node and browser DevTools.

The plus-operator +new Date() is FASTER ON OLD HOSTS, albeit slower on modern browsers. But we only care for performance on old hosts here, because modern ones use Date.now already.

if (typeof setTimeout!=='undefined') {
  var runAndAgain = function() {
    setTimeout(function() {
      runMillion();
      runAndAgain();
    }, 1);
  };
  runAndAgain();
}
else {
  while (true) {
    runMillion();
  }
}

function runMillion() {
  var start = new Date(), tmp=0;
  for (var i = 0; i < 1000000; i++) { tmp = +new Date(); }
  var plusNewDate = new Date() - start;

  var start = new Date(), tmp=0;
  for (var i = 0; i < 1000000; i++) { tmp = new Date().getTime(); }
  var newDateGetTime = new Date() - start;

  (typeof WScript !== 'undefined' ? function(x) { WScript.Echo(x) } :
   typeof console !== 'undefined' ? function(x) { console.log(x) } :
   alert)('plus '+plusNewDate+'\tgetTime '+newDateGetTime+'\t ratio '+(((10000*plusNewDate/newDateGetTime)|0)/100)+'%');
}

profilerEvent = typeof onProfilerEvent === "function" && onProfilerEvent.profiler === true
? onProfilerEvent
: undefined;
markInternal = performance && performance.now ? performance.now : Date.now ? Date.now : () => new Date().getTime();
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. See comment below on new Date().getTime() performance (it should be +new Date() instead here).

  2. Also please use typeof performance!=='undefined' instead of just checking for truthiness. Otherwise you'll get ReferenceError on platforms that don't have it defined.

  3. Lastly, it's quite non-obvious whether performance identifier here resolves to global-scope performance or ts.performance. I can see you've got that declare const performance up at line 6 — but it feels like a trick, and it may come back biting as resolution rules change all the time.

    Perhaps a clearer solution would be to move the whole Date.now thing in together with other fallback-safe utils (forEach, contains, indexOf) in core.ts around line 84?

    namespace ts {
        export var preciseTime =
            typeof performance!=='undefined' && performance && performance.now ? performance.now :
            Date.now ? Date.now :
            () => +new Date();
    }

    That way rather than having defined and called markInternal here in performance.ts, you'd call ts.preciseTime() from core.ts. Also you would replace overly optimistic Date.now at shims.ts isCancellationRequested at line 426 and editorServices.ts resolveNamesWithLocalCache line 145

@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants