Closed
Bug 1364442
Opened 7 years ago
Closed 7 years ago
Profiler JSON is broken depending on the OS locale because float values (used for minor GC timing) can use the wrong decimal separator
Categories
(Core :: JavaScript Engine, defect, P1)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla55
| Tracking | Status | |
|---|---|---|
| firefox-esr52 | --- | unaffected |
| firefox53 | --- | unaffected |
| firefox54 | --- | unaffected |
| firefox55 | --- | fixed |
People
(Reporter: mstange, Assigned: sfink)
References
Details
(Keywords: regression)
Attachments
(2 files)
|
26.41 KB,
application/json
|
Details | |
|
5.12 KB,
patch
|
jonco
:
review+
|
Details | Diff | Splinter Review |
Excerpt of broken JSON:
> "Total":2058,353000,
> "CancelIonCompilations":0,000000,
> "TraceValues":0,000000,
|
||
Comment 1•7 years ago
|
||
|
|
||
Comment 2•7 years ago
|
||
My OS is french (my Firefox isn't though :) ). In french we use commas as decimal separators, so could this be a locale problem ?
|
|
||
Comment 3•7 years ago
|
||
Launching Firefox with LC_ALL=C makes it work.
|
||
Comment 4•7 years ago
|
||
Ditto bug 1364347 - LC_ALL=C fixes the problem there.
|
Reporter | |
Comment 6•7 years ago
|
||
I think this is a regression from the patch "Record minor GC timings in profiles" from bug 1322560. This code uses the float overload of JSONPrinter::property instead of the TimeDuration one: http://searchfox.org/mozilla-central/rev/cd8c561106d804e26bc09389f18f361846d005eb/js/src/gc/Nursery.cpp#511-512 http://searchfox.org/mozilla-central/rev/cd8c561106d804e26bc09389f18f361846d005eb/js/src/vm/JSONPrinter.cpp#175 And the float printer uses vnsprintf with %f to convert floats to strings: http://searchfox.org/mozilla-central/rev/cd8c561106d804e26bc09389f18f361846d005eb/js/src/vm/Printer.cpp#84 http://searchfox.org/mozilla-central/rev/cd8c561106d804e26bc09389f18f361846d005eb/mozglue/misc/Printf.cpp#280 In comparison, JSONWriter uses double_conversion::DoubleToStringConverter::EcmaScriptConverter(): http://searchfox.org/mozilla-central/rev/cd8c561106d804e26bc09389f18f361846d005eb/mfbt/JSONWriter.h#399-409
|
|
Reporter | |
Updated•7 years ago
|
Summary: Profiler JSON is broken depending on the OS locale because TimeDuration values can use the wrong decimal separator → Profiler JSON is broken depending on the OS locale because float values (used for minor GC timing) can use the wrong decimal separator
|
Assignee | |
Comment 7•7 years ago
|
||
Argh, sorry. The stuff I use for the major GCs handles this explicitly already, and I should have known better when I saw the %f. I will fix.
|
|
Assignee | |
Comment 8•7 years ago
|
||
MozReview-Commit-ID: 3hFObaoD094
Attachment #8867262 -
Flags: review?(jcoppeard)
|
|
Assignee | |
Updated•7 years ago
|
Assignee: nobody → sphink
Status: NEW → ASSIGNED
|
||
Updated•7 years ago
|
Attachment #8867262 -
Flags: review?(jcoppeard) → review+
|
||
Updated•7 years ago
|
status-firefox53:
--- → unaffected
status-firefox54:
--- → unaffected
status-firefox55:
--- → affected
Pushed by sfink@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ef32458f11b8 Remove more locale-sensitive JSON output paths, r=jonco
|
||
Comment 10•7 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/ef32458f11b8
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
|
||
Updated•7 years ago
|
status-firefox-esr52:
--- → unaffected
You need to log in
before you can comment on or make changes to this bug.
Description
•