Add support for column property in frameTable object and update gecko version to 12, processed version to 15#1180
Conversation
…ofiler version to 12, and processed profile version to 15.
…cko-12 and processed-15 versions. Fix bug in procesed-14.
…cko-12 and processed-15 versions. Fix bug in procesed-14.
Codecov Report
@@ Coverage Diff @@
## master #1180 +/- ##
=========================================
- Coverage 76.06% 75.77% -0.3%
=========================================
Files 144 144
Lines 9426 9337 -89
Branches 2298 2251 -47
=========================================
- Hits 7170 7075 -95
- Misses 2014 2015 +1
- Partials 242 247 +5
Continue to review full report at Codecov.
|
|
The gecko bug associated with this change is: https://bugzilla.mozilla.org/show_bug.cgi?id=785922 |
gregtatum
left a comment
There was a problem hiding this comment.
Cool, the changes look pretty reasonable to handle the incoming format. The biggest thing that needs to be fixed is the processed profile type definition.
It looks like the FrameTable in src/types/profile.js still needs to be updated to take into account the changes. This should cause type errors everywhere we manually generate a FrameTable, although we should probably do some manual searching to ensure that the types found all of the instances where we are manually recreating the FrameTable.
Example location: https://github.com/devtools-html/perf.html/blob/8cf90bac49e911ce6228d5721cba389eb462a54f/src/profile-logic/transforms.js#L757-L765
Also, I'd like to wait until both the Gecko patch and this one are both r+ed before merging. I'm not sure if you already have thought through and have a plan for the landing process.
Thanks!
| //the old value used by the "category" property (4). The new value for | ||
| //"category" will be 5. The upgrader needs to remap all of the old category | ||
| //indices to the new one, and delete the old property in each frame. | ||
| const oldSchemaCategoryIndex = 4; |
There was a problem hiding this comment.
I'm not sure it's strictly necessary to remap the schema, as the converter does not rely on the order of the values, however this probably still gives the same results.
| convertToVersionElevenRecursive(profile); | ||
| }, | ||
| [12]: profile => { | ||
| //This version will add column numbers to the JS functions and scripts. |
There was a problem hiding this comment.
Tiny style nit: Generally we have a space after the // in comments in this project.
… property when generating the frameTable object
gregtatum
left a comment
There was a problem hiding this comment.
Looks good to me! I did another manual audit of the frameTable locations just in case, and it looks like every case is handled.
I checked the status of the other bug, is there a patch for handling the devtools performance tool? I think the change will invalidate their regex, and that will need to be handled before we land this.
Once the Gecko patch(es) are completely handled we can land and then deploy.
|
@gregtatum When I took a look it seemed like the devtools performance panel is already capable of parsing the column number field, ,i.e.: https://searchfox.org/mozilla-central/source/devtools/client/performance/modules/logic/frame-utils.js#103. I did a couple manual tests as well and I think it looks like it's working as intended out of the box?
|
|
Oh nice! In that case, are you ready for me to merge? |
|
@gregtatum yup, thanks! |
The corresponding gecko change for this pull request will emit column numbers to the JS functions and scripts. Most of this will be encapsulated in the JS description string, but there is also a new property in the frameTable object on each thread called "column". For clarity, the new property will replace the old "category" schema index value so that it can exist right after the "line" property. The schema index for the "category" property will now be "5", instead of "4" to accommodate.
To support the above changes, the gecko profile version will be upgraded to 12 and the processed profile version will be upgraded to 15. Test cases are updated as necessary.