1210217 - Create some kind of abstraction to avoid adding null elements to the output json by hand

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

We have all this code like:

    if (aFrame.mCategory.isSome()) {
      if (aFrame.mLine.isNothing()) {
        mFrameTableWriter.NullElement(); // implementation
        mFrameTableWriter.NullElement(); // optimizations
        mFrameTableWriter.NullElement(); // line
      }
      mFrameTableWriter.IntElement(*aFrame.mCategory);	
    }

and it is fairly easy to forget to add null elements but we simultaneuosly have to add them all over the place everywhere.

I think we should have some kind of API like

    abstraction.writeIntElement(CATEGORY, *aFrame.mCategory);

where CATEGORY is a constant int specifying which element in the array it is supposed to be, and the abstraction takes care of adding null elements for us.

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

See bug 1209779 for one example of how easy it is to forget to add the nulls.

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Stop manually adding null entries to the profiler's JSON output

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=587917f7d43e

Comment 4

[Shu-yu Guo [:shu]]

Comment on attachment 8668217 [details] [diff] [review]
Stop manually adding null entries to the profiler's JSON output

Review of attachment 8668217 [details] [diff] [review]:
-----------------------------------------------------------------

Please fix/double check the hasTrackedOptimizations case.

The rest is a nice improvement in readability.

::: tools/profiler/core/ProfileEntry.cpp
@@ +201,5 @@
> +//     }
> +//     ... etc ...
> +class MOZ_RAII AutoArraySchemaWriter
> +{
> +  SpliceableJSONWriter& mJsonWriter;

nit: all-caps JSON

@@ +271,3 @@
>  
> +    AutoArraySchemaWriter writer(mWriter, mUniqueStrings);
> +    writer.StringElement(Schema::STRATEGY, JS::TrackedStrategyString(strategy));

Huh, I thought |EnumName::VAL| syntax was only valid for enum classes, not bare enums.

@@ +561,5 @@
>                                  ? "ion"
>                                  : "baseline");
>  
>      if (jitFrame.hasTrackedOptimizations()) {
> +      writer.FillUpTo(Schema::OPTIMIZATIONS);

I don't think this is right. This is the case of streaming a JIT frame. The schema is [location, impl, opts, line, category], and mUniqueStrings.WriteElement is used directly to write the location and the implementation above, so writer's mNextFreeIndex == 0, right? This seems like it would fill in 2 nulls.

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Stop manually adding null entries to the profiler's JSON output

Comment 6

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Stop manually adding null entries to the profiler's JSON output

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=63bed7c1f5e1

Comment 8

[Shu-yu Guo [:shu]]

Comment on attachment 8669945 [details] [diff] [review]
Stop manually adding null entries to the profiler's JSON output

Review of attachment 8669945 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/133773490619

Comment 10

[Guilherme Lima]

The patch landed yesterday with the wrong number - https://hg.mozilla.org/mozilla-central/rev/133773490619