Skip to content

Add compatibility for the new profile format version with relevantForJS.#1450

Merged
mstange merged 1 commit into
firefox-devtools:masterfrom
mstange:relevant-for-js
Nov 6, 2018
Merged

Add compatibility for the new profile format version with relevantForJS.#1450
mstange merged 1 commit into
firefox-devtools:masterfrom
mstange:relevant-for-js

Conversation

@mstange

@mstange mstange commented Nov 2, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

This PR contains the profile format changes for #1392 / bug 1500467 but does not have any user observable changes.

@mstange mstange self-assigned this Nov 2, 2018
@mstange mstange requested review from gregtatum and removed request for gregtatum November 2, 2018 21:01
@mstange

mstange commented Nov 2, 2018

Copy link
Copy Markdown
Contributor Author

Unsetting review, I found some things that need to be cleaned up.

@codecov

codecov Bot commented Nov 2, 2018 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #1450 into master will increase coverage by 0.08%.
The diff coverage is 98.18%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1450      +/-   ##
==========================================
+ Coverage   80.61%   80.69%   +0.08%     
==========================================
  Files         157      157              
  Lines       10748    10789      +41     
  Branches     2620     2623       +3     
==========================================
+ Hits         8664     8706      +42     
+ Misses       1885     1884       -1     
  Partials      199      199
Impacted Files Coverage Δ
src/profile-logic/profile-data.js 84.31% <ø> (ø) ⬆️
src/test/fixtures/profiles/timings-with-js.js 100% <ø> (ø) ⬆️
src/test/fixtures/profiles/call-nodes.js 100% <ø> (ø) ⬆️
src/profile-logic/transforms.js 89.09% <ø> (ø) ⬆️
src/test/fixtures/profiles/gecko-profile.js 100% <ø> (ø) ⬆️
src/test/fixtures/profiles/make-profile.js 97.29% <100%> (+0.01%) ⬆️
src/profile-logic/processed-profile-versioning.js 93.56% <100%> (+0.55%) ⬆️
src/profile-logic/gecko-profile-versioning.js 91.02% <100%> (+0.83%) ⬆️
src/profile-logic/import/chrome.js 93.66% <100%> (+0.04%) ⬆️
src/profile-logic/process-profile.js 87.11% <94.44%> (+0.22%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d77aba3...dac44af. Read the comment docs.

@mstange mstange force-pushed the relevant-for-js branch 3 times, most recently from 70c061c to cf8b08b Compare November 3, 2018 17:35
@mstange mstange requested a review from gregtatum November 3, 2018 17:36
@mstange

mstange commented Nov 3, 2018

Copy link
Copy Markdown
Contributor Author

I'm not a big fan of this format, to be honest. We now have piles of repeated "false" values in every funcTable. Maybe a better format would be a stackType column which subsumes both the isJS and the relevantForJS columns and has values like native, js, label-relevant-for-js, label-not-relevant-for-js. But I don't really want to make such a chance now. We can still change the format at any point in the future.

@mstange mstange mentioned this pull request Nov 4, 2018
Closed
gregtatum
gregtatum approved these changes Nov 5, 2018
View reviewed changes

@gregtatum gregtatum left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good to me, thanks.

Comment thread src/profile-logic/gecko-profile-versioning.js
// Profiles now have a relevantForJS property in the frameTable.
// This column is false on C++ and JS frames, and true on label frames that
// are entry and exit points to JS.
const domCallRegex = /^(get |set )?\w+(\.\w+| constructor)$/;

@gregtatum gregtatum Nov 5, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It would be nice to provide a few examples of the types of names these match.

@mstange mstange merged commit a4c2285 into firefox-devtools:master Nov 6, 2018
@mstange mstange deleted the relevant-for-js branch November 6, 2018 17:27
mstange added a commit that referenced this pull request Nov 6, 2018
@mstange
Deploy November 6 2018, number 2 (Merge PR #1450)
28590bd 
Includes:

 - Add compatibility for the new profile format version with the relevantForJS column.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gregtatum gregtatum gregtatum approved these changes

Assignees

@mstange mstange

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@mstange @gregtatum