Convert sample_groups into an array since it can contain multiple groups#2287
Conversation
canova
force-pushed
the
counter-sample-groups
branch
from
f1cc685 to
1e0c3ad
Compare
Codecov Report
@@ Coverage Diff @@
## master #2287 +/- ##
==========================================
- Coverage 86.14% 86.13% -0.02%
==========================================
Files 204 204
Lines 14974 15005 +31
Branches 3759 3763 +4
==========================================
+ Hits 12900 12924 +24
- Misses 1902 1909 +7
Partials 172 172
Continue to review full report at Codecov.
|
|
|
||
| const samples = counter.sampleGroups.samples; | ||
| const sampleGroups = counter.sampleGroups; | ||
| if (sampleGroups.length !== 1) { |
There was a problem hiding this comment.
We are checking this here but it's not possible to fail here actually, checking here just in case. Because we only send one element from gecko, and if we fail to send that element(send an empty array instad), we handle this before coming here and remove that counter all together.
There was a problem hiding this comment.
I'd like to challenge this assumption: this is the case now, but will it be the case in the future? For example with fission in the future, we could have different sampleGroups for one process. (we don't know :) ).
I'm not saying we should account for this now. But I think that the condition should be sampleGroups.length === 0 instead, and that we should throw because if that shouldn't happen we shouldn't silently ignore the problem.
There was a problem hiding this comment.
For example with fission in the future, we could have different sampleGroups for one process. (we don't know :) ).
Actually this is still very unlikely(impossible?) with fission. I don't think that should be the case for memory graph. Yes, it can be an assumption for counters in general. But for memory counters, it's impossible to have multiple sample group, even with fission. That one element in that array is hardcoded in Firefox memory counter code. Also in a very unlikely event, if we change that code to have multiple groups, then we'll need to change this file to make it compatible with that, since the assumption for memory graph is that there is only one sample group. And we should think/discuss how to display multiple sample groups.
But I think that the condition should be sampleGroups.length === 0 instead, and that we should throw because if that shouldn't happen we shouldn't silently ignore the problem.
Throwing an error seems too much to me. If Firefox didn't capture any counter samples, that shouldn't make the whole front-end crash. console.error would be better I think. Also I don't think we should change the code to account all the sample groups(which there are no other groups right now and it's very unlikely because of the reason I mentioned earlier). I think it's okay to have an if check for 0 element and then usage of sampleGroups[0]. What do you think?
There was a problem hiding this comment.
Throwing an error seems too much to me. If Firefox didn't capture any counter samples, that shouldn't make the whole front-end crash.
That's not what I suggest :)
I suggest that we throw here because that shouldn't happen here. Indeed we should never add a TrackMemoryGraph if Firefox didn't capture any counter samples, because we should have filtered them out earlier.
That's why I suggest to throw here: because that should never happen. As you said it's not possible to fail here actually :)
julienw
left a comment
julienw
left a comment
There was a problem hiding this comment.
Thanks for the patch!
I'm pushing the review result about the code now, but I haven't tested it manually and I'd like to do it properly. So I'm keeping the review request so that I do it tomorrow.
|
|
||
| const samples = counter.sampleGroups.samples; | ||
| const sampleGroups = counter.sampleGroups; | ||
| if (sampleGroups.length !== 1) { |
There was a problem hiding this comment.
I'd like to challenge this assumption: this is the case now, but will it be the case in the future? For example with fission in the future, we could have different sampleGroups for one process. (we don't know :) ).
I'm not saying we should account for this now. But I think that the condition should be sampleGroups.length === 0 instead, and that we should throw because if that shouldn't happen we shouldn't silently ignore the problem.
canova
left a comment
canova
left a comment
There was a problem hiding this comment.
Thanks for the review Julien! I addressed most of the reviews and added some comments for the others. Could you please take a look again?
|
|
||
| const samples = counter.sampleGroups.samples; | ||
| const sampleGroups = counter.sampleGroups; | ||
| if (sampleGroups.length !== 1) { |
There was a problem hiding this comment.
For example with fission in the future, we could have different sampleGroups for one process. (we don't know :) ).
Actually this is still very unlikely(impossible?) with fission. I don't think that should be the case for memory graph. Yes, it can be an assumption for counters in general. But for memory counters, it's impossible to have multiple sample group, even with fission. That one element in that array is hardcoded in Firefox memory counter code. Also in a very unlikely event, if we change that code to have multiple groups, then we'll need to change this file to make it compatible with that, since the assumption for memory graph is that there is only one sample group. And we should think/discuss how to display multiple sample groups.
But I think that the condition should be sampleGroups.length === 0 instead, and that we should throw because if that shouldn't happen we shouldn't silently ignore the problem.
Throwing an error seems too much to me. If Firefox didn't capture any counter samples, that shouldn't make the whole front-end crash. console.error would be better I think. Also I don't think we should change the code to account all the sample groups(which there are no other groups right now and it's very unlikely because of the reason I mentioned earlier). I think it's okay to have an if check for 0 element and then usage of sampleGroups[0]. What do you think?
| } | ||
| } | ||
| } | ||
| ], |
There was a problem hiding this comment.
I added this test inside gecko-2.json because that comment says so 😄
profiler/src/test/unit/profile-upgrading.test.js
Lines 134 to 135 in bb0caf0
But I think(if I remember correctly) previously we had a JSON file for each upgraders and then we decided to reduce those because of a few reasons.
- It takes a lot of time to run the test.
- It's a bit painful to add those tests for each upgraders.
I see your reasoning but I think it's okay to lie here a bit, because counters don't affect any other places in the upgrading/processing part and if we start to increment that number again, I'm afraid that we are gonna start having an upgrader for each version again.(not that I researched but I think new features tend to change more than the old features at first usually)
For example I had to create a processed-3.json for page information because it was affecting other markers and their upgraders. But this seems harmless here.
So I would prefer keeping it here if that's okay. What do you think?
| } | ||
| ] | ||
| }, | ||
| "counters": [ |
There was a problem hiding this comment.
preprocessedProfileVersion is 24 here.
julienw
left a comment
julienw
left a comment
There was a problem hiding this comment.
Thanks, this looks good !
I still would like the throws in TrackMemory instead of all console.error because if we enter these if conditions this means that we have a bug in some other code.
If we throw this can be caught in tests, and otherwise in runtime we'll see very quick that we did a mistake. On the contrary with the error (even with a console.error) we might miss it.
Again, I advise for this throw because this should never happen. If this could happen then this would be another story.
I hope this is clearer!
canova
force-pushed
the
counter-sample-groups
branch
from
c17bfa5 to
722f504
Compare
|
@julienw thanks for the review. Yeah, that makes sense. Changed the code to throw errors instead. |
2 participants
This is the front-end changes of Bug 1584190. Because of the bug on gecko side sample_groups was an object instead of an array. This changes it to an array. Malloc counters can have only one sample groups, that's why other items in the array is ignored for memory tracks only.
cc @squelart