Closed Bug 1165410 Opened 5 years ago Closed 5 years ago

Replace the StatisticsSerializer's JSON output

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: terrence, Assigned: terrence)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

The dual-purposing of the console and json output via StatisticsSerializer is by far the worst code I've ever written, certainly for Mozilla. I'm profoundly sorry to everyone that has had to interact with it, or its output, in the last 3 years. :-(

Despite the endless series of bugs when I first implemented it -- which should have had me rewriting it at the time -- it all more or less works now. Actually, the emphasis should be on the less *less*. In rewriting it, I found two bugs:

First the float output was totally wrong. This shouldn't require any fixing in dependent code, it will just make the numbers more precise to +/-1 microsecond instead of +/-1 millisecond.

Secondly, the automatic replacement we did of '-' with 'removed' for "Chunks -" resulted in "Mark Runtime-wide data" going off the rails. This now has the sane name of "mark_runtime_wide_data" and not "mark_runtimeremoved_wide_data".

This second might require some work for users of the interface; otherwise the output is identical, modulo a few spacing changes.

As part of this patch I tried to strip the asJSON_ flag and other dependent code out of StatisticsSerializer, but the crazy just goes too deep. At this point I'd like to replace the compact notation too and then salt and burn the old infrastructure.
Attachment #8606404 - Flags: review?(sphink)
See Also: → 748397
Comment on attachment 8606404 [details] [diff] [review]
cleanup_json_formatting-v0.diff

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

::: js/src/gc/Statistics.cpp
@@ +85,5 @@
>      void appendDecimal(const char* name, const char* units, double d) {
>          if (d < 0)
>              d = 0;
>          if (asJSON_)
> +            appendNumber(name, "%d.%03d", units, (int)d, (int)(d * 1000.) % 1000);

As long as you're touching things, C++-style «int(d)» casts.

@@ +910,5 @@
> +
> +        UniqueChars name = FilterJsonKey(phases[phase].name);
> +        int64_t ownTime = phaseTimes[dagSlot][phase];
> +        JS_snprintf(buffer, sizeof(buffer), "\"%s\":%lu.%03lu",
> +                    name.get(), ownTime / 1000, ownTime % 1000);

Why do you get to use %lu and avoid casting the args here and formatSliceDescription but not appendDecimal?
Attachment #8606404 - Flags: review?(sphink) → review+
(In reply to Steve Fink [:sfink, :s:] from comment #1)
> Comment on attachment 8606404 [details] [diff] [review]
> cleanup_json_formatting-v0.diff
> 
> Review of attachment 8606404 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/gc/Statistics.cpp
> @@ +85,5 @@
> >      void appendDecimal(const char* name, const char* units, double d) {
> >          if (d < 0)
> >              d = 0;
> >          if (asJSON_)
> > +            appendNumber(name, "%d.%03d", units, (int)d, (int)(d * 1000.) % 1000);
> 
> As long as you're touching things, C++-style «int(d)» casts.
> 
> @@ +910,5 @@
> > +
> > +        UniqueChars name = FilterJsonKey(phases[phase].name);
> > +        int64_t ownTime = phaseTimes[dagSlot][phase];
> > +        JS_snprintf(buffer, sizeof(buffer), "\"%s\":%lu.%03lu",
> > +                    name.get(), ownTime / 1000, ownTime % 1000);
> 
> Why do you get to use %lu and avoid casting the args here and
> formatSliceDescription but not appendDecimal?

Because appendDecimal is dead code now? I guess I could probably fold in the removal of that. I have it as a separate patch right now.
Note, I only touched up appendDecimal so that I could compare the old and new output without every line being off because of the broken old formatting.
Err, sorry, this is the JSON formatter; append decimal is only dead after adding the compact output as well, although I would like to land those three in sync.
https://hg.mozilla.org/mozilla-central/rev/1a955124eccc
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Depends on: 1170039
You need to log in before you can comment on or make changes to this bug.