1584190 - Profiler counter samples are stored in a single object instead of an array of objects

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect, P1)

Description

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

First reported in https://github.com/firefox-devtools/profiler/issues/2248 :

From the code in https://searchfox.org/mozilla-central/rev/45f30e1d19bde27bf07e47a0a5dd0962dd27ba18/tools/profiler/core/ProfileBufferEntry.cpp#1569 it looks like this is possible that sample_groups is an empty object.
Actually we may miss an array opening in this code: according to the doc in https://searchfox.org/mozilla-central/rev/45f30e1d19bde27bf07e47a0a5dd0962dd27ba18/tools/profiler/core/ProfileBufferEntry.h#539-559, sample_groups should be an array of object, but it's only an object. I believe there is a mistake in this serialization...

Comment 1

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1584190 - In JSON profile, counters' sample_groups should be an array of objects - r?canaltinova

profile.counters[n].sample_groups was mistakenly streamed as an object, which
prevents having more than one, and goes against the published format
documentation.

The front-end was implemented to process the incorrect format, so it will need
to be updated as well; hence the version change to 18.

Comment 2

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Nazim, this will require a front-end version change first, could you please handle it? (You'll be much faster than I can be!)

The attached format change is in counters[n].sample_groups, which was one object:

{ "counters": [ { "sample_groups": { "id": 0, "samples": {...} } } ]
                                   ^ one object only!

And now is an array of objects, allowing more than one:

{ "counters": [ { "sample_groups": [ { "id": 0, "samples": {...} }, { "id": 0, "samples": {...} } ] } ]
                             array ^ ^ 1st object                   ^ 2nd object (or more)        ^ end array

I'm guessing you'll need to change both the processor to translate arrays, and the UI to handle these arrays.

Comment 3

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Created https://github.com/firefox-devtools/profiler/pull/2287 for this. We should land the github side first to be able to handle this change.

Comment 4

[Julien Wajsberg [:julienw]]

Hey Gerald, can you please write a link to a linux try build so that I can download a build for my platform and try the profiler patch with this?
Thanks!

Comment 5

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

I was going to say "it should already be in my Try collection!" but of course, this one time it's not! Sorry 😅

Here it is now:
https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=1982cc7a5206922bc960c0379a622a909ac1e4cd

Thank you for trying to test it.

Comment 6

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Front-end side is landed and deployed. It's good to land this patch now.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3c0094a58673
In JSON profile, counters' sample_groups should be an array of objects - r=canaltinova

Comment 8

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/3c0094a58673

Comment 9

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3c0094a58673