1490821 - Use JSONPrinter for PCCount profiler

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P2)

Description

[André Bargull [:anba]]

While working on bug 1482846 and bug 1485066, I saw that the PCCount profiler code could be updated to use the |JSONPrinter| class instead of doing its adhoc JSON processing.


(Note: I have no idea if the PCCount profiler is actually still used.)

Comment 1

[André Bargull [:anba]]

Attachment: bug1490821.patch

Updates the PCCount profiler to use JSONPrinter and adds some testing functions for the profiler. 

vm/BytecodeUtil.cpp
- Change the PCCount profiler functions to use JSONPrinter, which allows to remove quite a bit of code and makes it more readable, too. 

vm/JSONPrinter.{h,cpp}
- Add an option to omit indentation, because (1) this matches the previous PCCount profiler output and (2) it makes the JSON output smaller, which is probably necessary for the PCCount profiler output, because it can become quite large.
- Add JSONPrinter::beginList() to start a new JSON array.
- And improve the performance of the JSONPrinter by replacing printf() calls with putChar() resp. put() where applicable.

vm/Printer.{h,cpp}
- Add a helper to emit a correctly JSON quoted string.

vm/StringType.{h,cpp}
- Make js::StringToSource a static function, because it's no longer called outside of StringType.cpp.

builtin/TestingFunctions.cpp
- Add testing functions for the PCCounter profiler. These are not fuzzing safe, because the profiler uses PersistentRooted variables (see JSRuntime::scriptAndCountsVector), which makes it necessary to manually clean up the profiler data before destroying the runtime.

jit-test/tests/profiler/pc-count-profiler.js
- And add a simple smoke test for the PCCount profiler API.

Comment 2

[Nicolas B. Pierron [:nbp]]

(In reply to André Bargull [:anba] from comment #0)
> (Note: I have no idea if the PCCount profiler is actually still used.)

If not, we should remove it instead. I know Jan used it a few years ago with the -B option of the JS shell.

Comment 3

[Jan de Mooij [:jandem]]

(In reply to Nicolas B. Pierron [:nbp] from comment #2)
> If not, we should remove it instead. I know Jan used it a few years ago with
> the -B option of the JS shell.

I've used -D to see basic block counts for Ion but I don't think that uses the JSON printer?

Comment 4

[Ted Campbell [:tcampbell]]

Comment on attachment 9008559 [details] [diff] [review]
bug1490821.patch

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

Excellent work as usual.

::: js/src/builtin/TestingFunctions.cpp
@@ +6319,5 @@
>              return false;
>          }
> +
> +        RootedObject pccount(cx, JS_NewPlainObject(cx));
> +        if (!pccount || !JS_DefineProperty(cx, obj, "pccount", pccount, 0)) {

Break into two ifs since JS_DefineProperty has side-effects.

::: js/src/vm/Printer.cpp
@@ +317,3 @@
>      using CharPtr = mozilla::RangedPtr<const CharT>;
>  
> +    const char* escapeMap = target == QuoteTarget::String ? js_EscapeMap : JSONEscapeMap;

Parens around the (target == QuoteTarget::String) for readability.

Comment 5

[André Bargull [:anba]]

Attachment: bug1490821.patch

Updated per review comments, carrying r+.

Comment 6

[André Bargull [:anba]]

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fadacb1b8dcb52587b5758d2ed0152cea31ed507
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5fc89c159ca902ef1d6404c238570258750356c

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/89349ce25fe3

Comment 8

[Cristian Brindusan [:cbrindusan]]

Backed out changeset 89349ce25fe3 (Bug 1490821) for jittest failures on pc-count-profiler.js.

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/2e01025ece1b913b7a92a446e8bdfcfdb1430b7f

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&revision=89349ce25fe3e40fb2805a1aeaeda86e00374f6c&selectedJob=208375032

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=208375032&repo=mozilla-inbound&lineNumber=79687

Comment 9

[André Bargull [:anba]]

Attachment: bug1490821.patch

Updated to also JSON-quote the file name to ensure backslashes are correctly escaped on Windows.

Comment 10

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0ad60ce61c6ac6af8b86bb664f2ff2c968d8e22d

Comment 11

[Natalia Csoregi [:nataliaCs]]

Pulsebot is down at the moment.

Land: https://hg.mozilla.org/integration/mozilla-inbound/rev/719c199e639882dd0355bbac2e395a1eaeb70981

Removing checkin-needed.

Comment 12

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/719c199e6398