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

RESOLVED FIXED in Firefox 14

Status

()

Core
JavaScript Engine
--
major
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: sfleiter, Assigned: terrence)

Tracking

14 Branch
mozilla15
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox14+ fixed, firefox15+ fixed)

Details

(URL)

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

5 years ago
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?
(Assignee)

Comment 2

5 years ago
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
(Assignee)

Updated

5 years ago
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
(Assignee)

Comment 3

5 years ago
Created attachment 617942 [details] [diff] [review]
v0

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.
(Assignee)

Comment 5

5 years ago
(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.
(Assignee)

Comment 6

5 years ago
Created attachment 617951 [details] [diff] [review]
v1: Not as bad as I thought it would be.

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)
(Assignee)

Comment 7

5 years ago
Created attachment 617957 [details] [diff] [review]
v2: Even better.

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+
(Assignee)

Comment 9

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec2a64a2f5d9
This breaks all tools which depend on the API in Firefox 14 and 15. Asking for tracking both branches.
status-firefox14: --- → affected
status-firefox15: --- → affected
tracking-firefox14: --- → ?
tracking-firefox15: --- → ?
Version: 13 Branch → 14 Branch
https://hg.mozilla.org/mozilla-central/rev/ec2a64a2f5d9
Status: ASSIGNED → RESOLVED
Last Resolved: 5 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.
tracking-firefox14: ? → +
tracking-firefox15: ? → +
status-firefox15: affected → fixed
(Reporter)

Comment 14

5 years ago
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?
status-firefox15: fixed → affected
Flags: in-testsuite?
(Assignee)

Comment 16

5 years ago
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.
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.
(Assignee)

Comment 18

5 years ago
Created attachment 618821 [details] [diff] [review]
v1: Using int.

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+
(Assignee)

Comment 19

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/58a6a61544c9
https://hg.mozilla.org/mozilla-central/rev/58a6a61544c9
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 21

5 years ago
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!
(Reporter)

Comment 22

5 years ago
This finally works for me (again).
Thanks a lot for your work!

Updated

5 years ago
status-firefox15: affected → fixed
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.
(Assignee)

Comment 26

5 years ago
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.
(Assignee)

Comment 28

5 years ago
Created attachment 628006 [details] [diff] [review]
Combined and backported patches to aurora.

[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+
(Assignee)

Comment 30

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/91eca10ae9a5
(Assignee)

Updated

5 years ago
status-firefox14: affected → fixed
(Assignee)

Updated

2 years ago
See Also: → bug 1165410
You need to log in before you can comment on or make changes to this bug.