Closed Bug 748397 Opened 9 years ago Closed 9 years ago

JSON from garbage-collector activity tracking API (bug 531396) is broken with comma as fraction separator

Categories

(Core :: JavaScript Engine, defect)

14 Branch
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla15
Tracking Status
firefox14 + fixed
firefox15 + fixed

People

(Reporter: sfleiter, Assigned: terrence)

References

()

Details

Attachments

(3 files, 3 obsolete files)

JSON from garbage-collector activity tracking API is broken with comma as fraction separator.

The API should not act Locale dependent and not produce broken JSON.

See https://github.com/mozilla/memchaser/issues/132

Example:
Timestamp: 24.04.2012 17:38:06
Error: {"timestamp": 1335281886926635, "total_time": 106,6, "compartments_collected": 300, "total_compartments": 300, "mmu_20ms": 0, "mmu_50ms": 0, "max_pause": 106,6, "nonincremental_reason": "GC mode", "allocated": 81, "added_chunks": 30, "removed_chunks": 0, "Slices": [{"slice": 0, "time": 106,6, "pause": 106,6, "reason": "POST_COMPARTMENT", "times": {"begin_callback": 0,1, "wait_background_thread": 0,0, "purge": 1,2, "mark": 49,9, "mark_roots": 7,5, "mark_delayed": 0,0, "mark_other": 3,5, "finalize_start_callback": 0,7, "sweep": 42,8, "sweep_compartments": 13,7, "sweep_object": 12,9, "sweep_string": 1,9, "sweep_script": 1,1, "sweep_shape": 6,8, "discard_code": 1,2, "discard_analysis": 3,2, "discard_ti": 0,2, "sweep_types": 0,4, "clear_script_analysis": 0,2, "finalize_end_callback": 4,7, "deallocate": 0,1, "end_callback": 12,2}}], "totals": {"begin_callback": 0,1, "wait_background_thread": 0,0, "purge": 1,2, "mark": 49,9, "mark_roots": 7,5, "mark_delayed": 0,0, "mark_other": 3,5, "finalize_start_callback": 0,7, "sweep": 42,8, "sweep_compartments": 13,7, "sweep_object": 12,9, "sweep_string": 1,9, "sweep_script": 1,1, "sweep_shape": 6,8, "discard_code": 1,2, "discard_analysis": 3,2, "discard_ti": 0,2, "sweep_types": 0,4, "clear_script_analysis": 0,2, "finalize_end_callback": 4,7, "deallocate": 0,1, "end_callback": 12,2}}
Source File: resource://memchaser-at-quality-dot-mozilla-dot-org/api-utils/lib/cuddlefish.js -> resource://memchaser-at-quality-dot-mozilla-dot-org/memchaser/lib/garbage-collector.js
Line: 91
Stefan, what API are you using to get this json?
This is my bad.  I didn't think that spidermonkey's low-level snprintf implementation would have locale support.  Will try to whip something up today to hack around the problem.
Assignee: nobody → terrence
Status: NEW → ASSIGNED
Severity: normal → major
Component: XPCOM → JavaScript Engine
OS: Linux → All
QA Contact: xpcom → general
Hardware: x86_64 → All
Target Milestone: --- → mozilla14
Version: unspecified → 13 Branch
Attached patch v0 (obsolete) — Splinter Review
This also removes locale encoding for the console messages.  This is a shame, but I don't think it is enough of one to complexify the code in such a low-level tool.
Attachment #617942 - Flags: review?(wmccloskey)
Instead of this, could you add a new method to StatisticsSerializer like this:

  void appendDecimal(const char *name, const char *units, double n) {
    if (asJSON_)
      appendNumber(name, "%.1F", units, n);
    else
      appendNumber(name, "%.1f", units, n);
  }

and use this for all the existing %.1f stuff. That way the error console output will be localized.

Also, I don't think we need to change the GC(T+xxx) stuff. It never goes to JSON.
(In reply to Bill McCloskey (:billm) from comment #4)
> Also, I don't think we need to change the GC(T+xxx) stuff. It never goes to
> JSON.

True.  That was just to make the output consistent.
Not as bad as I thought it would be.  I was worried about appendIfNonZero, but that can just be a second branch.
Attachment #617942 - Attachment is obsolete: true
Attachment #617942 - Flags: review?(wmccloskey)
Attachment #617951 - Flags: review?(wmccloskey)
Attached patch v2: Even better.Splinter Review
Bill pointed out that since we're hard-coding the format string we shouldn't be taking varargs anymore.  This allows us to just use appendDecimal directly from appendIfNonZeroMS, simplifying things greatly.
Attachment #617951 - Attachment is obsolete: true
Attachment #617951 - Flags: review?(wmccloskey)
Attachment #617957 - Flags: review?(wmccloskey)
Comment on attachment 617957 [details] [diff] [review]
v2: Even better.

Thanks. Can you move the double argument of appendDecimal last so the order is more like appendNumber?
Attachment #617957 - Flags: review?(wmccloskey) → review+
This breaks all tools which depend on the API in Firefox 14 and 15. Asking for tracking both branches.
Version: 13 Branch → 14 Branch
https://hg.mozilla.org/mozilla-central/rev/ec2a64a2f5d9
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: mozilla14 → mozilla15
Is memchaser still broken. I don't get any gc values, cc is ok.
I'm using an up-to-date m-c build.
(In reply to Olli Pettay [:smaug] from comment #12)
> Is memchaser still broken. I don't get any gc values, cc is ok.
> I'm using an up-to-date m-c build.

You could add a dump statement in Memchaser to test the data as coming from the API. See lib/garbage_collector.js line 91.
Sorry, but this is not fixed
(Mozilla/5.0 (X11; Linux x86_64; rv:15.0) Gecko/20120426 Firefox/15.0a1)

The JSON structure looks like this now:

{"timestamp": 1335439002064508, "total_time": %F, "compartments_collected": 342, "total_compartments": 342, "mmu_20ms": 0, "mmu_50ms": 0, "max_pause": %F, "nonincremental_reason": "GC mode", "allocated": 98, "added_chunks": 2, "removed_chunks": 2, "Slices": [{"slice": 0, "time": %F, "pause": %F, "reason": "CC_WAITING", "times": {"begin_callback": %F, "wait_background_thread": %F, "purge": %F, "mark": %F, "mark_roots": %F, "mark_delayed": %F, "mark_other": %F, "finalize_start_callback": %F, "sweep": %F, "sweep_compartments": %F, "sweep_object": %F, "sweep_string": %F, "sweep_script": %F, "sweep_shape": %F, "discard_code": %F, "discard_analysis": %F, "discard_ti": %F, "sweep_types": %F, "clear_script_analysis": %F, "finalize_end_callback": %F, "deallocate": %F, "end_callback": %F}}], "totals": {"begin_callback": %F, "wait_background_thread": %F, "purge": %F, "mark": %F, "mark_roots": %F, "mark_delayed": %F, "mark_other": %F, "finalize_start_callback": %F, "sweep": %F, "sweep_compartments": %F, "sweep_object": %F, "sweep_string": %F, "sweep_script": %F, "sweep_shape": %F, "discard_code": %F, "discard_analysis": %F, "discard_ti": %F, "sweep_types": %F, "clear_script_analysis": %F, "finalize_end_callback": %F, "deallocate": %F, "end_callback": %F}}

I would assume that %F should be replaced by a float value with dot as the fraction separator.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Can we get some kind of unit tests here or for the whole API?
Flags: in-testsuite?
It turns out the %F format flag is not in SuSv2.  This patch re-does the printing in a hackier, but portable, way.
Attachment #618806 - Flags: review?(wmccloskey)
(In reply to Terrence Cole [:terrence] from comment #16)
> Created attachment 618806 [details] [diff] [review]
> v0: Use only standard format flags.
> 
> It turns out the %F format flag is not in SuSv2.  This patch re-does the
> printing in a hackier, but portable, way.

I don't think this will work on 32-bit machines, where a long is 32 bits. I would suggest just using %d and casting to an int in both cases. These numbers are never going to be very big.
Attached patch v1: Using int.Splinter Review
Verified in a 32bit browser build.
Attachment #618806 - Attachment is obsolete: true
Attachment #618806 - Flags: review?(wmccloskey)
Attachment #618821 - Flags: review?(wmccloskey)
Attachment #618821 - Flags: review?(wmccloskey) → review+
https://hg.mozilla.org/mozilla-central/rev/58a6a61544c9
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Stephan, sorry for the continued bustage.  Since we don't yet have tests, can you confirm that this works?  This went into central early this morning, so it should be in tonight's nightly build.  Thanks!
This finally works for me (again).
Thanks a lot for your work!
Please nominate for Aurora approval so we can uplift.
(In reply to Lukas Blakk [:lsblakk] from comment #23)
> Please nominate for Aurora approval so we can uplift.

Doesn't 'tracking-firefox14:+' mean we can directly check-in the patch? Sorry if I miss something.
tracking-X+ doesn't mean that the patch can land.
This had fallen off of my radar.  What action is needed to get this bug fixed in firefox14?
Go to details for the patch you want to land on Aurora, select ? under approval-mozilla-aurora, fill out the form, wait for approval.
[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
The extension "MemChaser" will fail to work depending on what the user's LC_NUMERIC environment variable is set to.

User impact if declined: 
Memchaser will stop working when users upgrade from Firefox13 to Firefox14.

Testing completed (on m-c, etc.): 
This patch has been in Nightlys since April, fixing the bug and causing no other regressions.

Risk to taking this patch (and alternatives if risky): 
Low risk. The code is only used by one extension and is now well tested.

String or UUID changes made by this patch: 
None.
Attachment #628006 - Flags: approval-mozilla-aurora?
Comment on attachment 628006 [details] [diff] [review]
Combined and backported patches to aurora.

low risk, fixes tools, approved.
Attachment #628006 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
See Also: → 1165410
You need to log in before you can comment on or make changes to this bug.