Change the UI to use sample counts, and add explicit support for traced profiles #2558
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2558 +/- ##
==========================================
- Coverage 86.53% 86.40% -0.14%
==========================================
Files 217 217
Lines 17256 17370 +114
Branches 4475 4501 +26
==========================================
+ Hits 14933 15008 +75
- Misses 2124 2162 +38
- Partials 199 200 +1
Continue to review full report at Codecov.
|
|
|
|
@tehmatt An explicit goal here is to make our infrastructure support tracing formats better. |
gregtatum
force-pushed the
gregtatum:sample-counts
branch
4 times, most recently
from
3ca2a5c
to
f35fb4e
gregtatum
changed the title
|
Thanks for this work, this is impressive! I'm glad you're looking at what we can do with the sidebar because my initial implementation was just... initial and it was always thought that we could improve it with some time in our hands. I'm requesting changes mainly for the algorithm for the traced timing which (I think) lacks support for inverted trees and would be better implemented in some existing location (either timings computations or call tree computations). This patch does a few separate things:
I understand you want traced timings with the move to sample counts to keep the "ms" information. But maybe that could wait. Therefore I think it would be beneficial if you can split the PR in these 4 parts. Then I can probably r+ some of these PRs easily and we can look at the traced timings more closely. |
|
Ok, I handled all of the inline comments except for the algorithm on the timing, and the splitting out of the tooltips persisting. I'm not re-requesting for review quite yet, but I'm happy to take more comments. I need to figure out if and how to split up the PR, since I'd like to deploy this chunk all together since it's changing the meaning of a core metric. We can chat about this tomorrow. |
|
@julienw I just re-requested for review. I initially attempted to pull out the TracedTiming work into a new PR, but I realized that there is a hard requirement on the weights and |
|
Thanks for the patch! I found a few more issues, and also added a few suggestions for tests and other things, some of them for follow-ups. But I'm still approving, trusting that you'll look into the problems before landing :-) I'll still be happy to have a last look if you want to! Thanks! |
I ended up doing this because of bugs of not properly copying the weight type when generating a new sample table. This seemed like the safer option.
This new approach uses the existing mechanisms in the call tree code to compute the traced timing. There is a small amount of code duplication, but it is kept to a minimum. I felt this was justified to keep the implementations simple and fast.
gregtatum
merged commit c4f68f4
into
firefox-devtools:master
This resolves #813 by creating the idea of different types of weights for our sample data.
Ok, this is a big PR. Everything is split out into logical commits. The reason it's so big is that changing to sample counts really changes how we interpret everything in the UI. I'm not sure what the best way to review this is, especially as GitHub makes this type of review kind of difficult. Maybe we can chat synchronously about the best way to approach it.
There is a small regression to our code coverage, this is mostly the branches to
tracing-ms. I don't have any data actively targeting this weight type, so it's a bit difficult. I could do it with JS tracer information, but it seems a bit worthless since that data source is not enabled.Deploy preview with a fast sampling interval
My preference here for the review would be to keep in scope any bugs, code quality and architectural issues. I would prefer to file follow-ups for additional polish features on the UI, especially as this is a rather large PR. I have some additional follow-ups I'd like to do after this. This includes: