perf(perf-timer): add instrumentation for Rule.runChecks #701
Conversation
|
|
|
@paulirish you can get rid of the JSHint errors by hiking the var definitions up to the top of the function. |
thanks, done. |
|
As an FYI - the async code only runs when you are analyzing pages with iframes, so for the purposes of most perf testing, this code should be sufficient. Thanks for the contribution!! |
|
@WilcoFiers @marcysutton do we care about the commit log subject being "perf:"? We haven't used this before but I am ok with us adopting this going forward - WDYT? |
|
|
|
|
|
@WilcoFiers this just adds the rule information to the performance timer functionality |
|
retitled as "chore(performance): ". sgty? |
With performanceTimer on, we get some nice usertiming measures added. But in #701, some _jerk_ picked an endpoint for rules that I, personally, think can be improved. :) This moves the end mark of each rule to when the checks have synchronously finished. This change means we don't include the asynchronous bit afterwards, but that async bit is the subject of #1172. For accurate timing, it makes more sense to keep these measures synchronous. It also makes reading the flame chart more logical. Screenshots of before and after:  ## Reviewer checks **Required fields, to be filled out by PR reviewer(s)** - [x] Follows the commit message policy, appropriate for next version - [x] Has documentation updated, a DU ticket, or requires no documentation change - [x] Includes new tests, or was unnecessary - [x] Code is reviewed for security by: @WilcoFiers
When running things with perfTimer, I noticed sometimes there was significant cost within the runChecks methods, however that wasn't always obvious from the existing performanceTimer measures.
This adds instrumentation around the
runChecksmethods.One note is that the intrumentation is synchronous, however checks finish asynchronously. But in my experience, it appears the majority of the cost happens in the first bit of synchronous runChecks work. So this seems OK.
Screenshot including the nontrivial cost of
runchecks-duplicate-id:This patch doesn't pass JSHint. I'm interested in your suggestions on handling that.