1491909 - Allow the use of mozilla::JSONWriter in SpiderMonkey

Status: RESOLVED INACTIVE

(Core :: JavaScript Engine, defect, P2)

Description

[Denis Palmeiro [:denispal]]

It is not currently possible to use the mozilla:JSONWriter class within SpiderMonkey.  Most of the methods will allocate an internal buffer via mozilla::MakeUnique (https://searchfox.org/mozilla-central/source/mfbt/JSONWriter.h#192) and this will fail the check_vanilla_allocations.py build check test:

TEST-UNEXPECTED-FAIL | check_vanilla_allocations.py | 'operator new[](unsigned' present in TraceLogging.o

It's currently blocking bug 1480104, since I cannot output any of the TraceLogger information to the JSONWriter object used by the Gecko profiler.

Comment 1

[Denis Palmeiro [:denispal]]

Hi Steve, here is the bug we discussed in #jsapi.  Thanks!

Comment 2

[Steve Fink [:sfink] [:s:]]

My understanding is that the restriction comes about because of https://searchfox.org/mozilla-central/source/config/check_vanilla_allocations.py#7-10

So we wouldn't want to just whitelist JSONWriter's usage if we still want to support JS_USE_CUSTOM_ALLOCATOR.

One fix would be to pull js/src/vm/MallocProvider.h stuff into mfbt and give JSONWriter an AllocPolicy.

I don't know what the use cases of JS_USE_CUSTOM_ALLOCATOR are, in order to judge whether these JSON strings need to respect it. But I see some Waldo tracks in this area, so I'm going to needinfo him.

Comment 3

[André Bargull [:anba]]

(In reply to Steve Fink [:sfink] [:s:] from comment #2)
> I don't know what the use cases of JS_USE_CUSTOM_ALLOCATOR are, in order to
> judge whether these JSON strings need to respect it. But I see some Waldo
> tracks in this area, so I'm going to needinfo him.

I'm not even sure JS_USE_CUSTOM_ALLOCATOR is still functional, because the else-block [1] contains definitions which are necessary to compile SpiderMonkey. 

[1] https://searchfox.org/mozilla-central/rev/6c82481caa506a240a626bb44a2b8cbe0eedb3a0/js/public/Utility.h#52-437


Apart from the custom allocator issue, SpiderMonkey also has this class [2], which may be sufficient for outputting JSON code.

[2] https://searchfox.org/mozilla-central/source/js/src/vm/JSONPrinter.h

Comment 4

[Denis Palmeiro [:denispal]]

> Apart from the custom allocator issue, SpiderMonkey also has this class [2],
> which may be sufficient for outputting JSON code.
> 
> [2] https://searchfox.org/mozilla-central/source/js/src/vm/JSONPrinter

This might be the last resort, bridging JSONPrinter to write into a buffer held by JSONWriter.   I was hoping there may be a simpler solution that lets us use JSONWriter directly though.  The profiler has a few classes that inherit from JSONWriter, so it would be nice to line up and use the same routines with how the rest of the profile is written.

Comment 5

[Denis Palmeiro [:denispal]]

Attachment: Add an AllocPolicy to mozilla::JSONWriter

JSONWriter currently calls new and delete indirectly through mozilla::MakeUnique to allocate a buffer.  Becuase of this, the methods of this class cannot be invoked within Spidermonkey due to https://searchfox.org/mozilla-central/source/config/check_vanilla_allocations.py#6-14.  Therefore, JSONWriter needs an AllocPolicy template parameter so that the allocation and deallocation routines can be changed to match the JS AllocPolicy when invoked within SpiderMonkey.

Comment 6

[Denis Palmeiro [:denispal]]

Discussed this with Jeff in #jsapi.  The approach we'll try is to add an AllocPolicy to JSONWriter, and then change it to the JS AllocPolicy when needed for use within SpiderMonkey.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bee1c344cb47
Add an AllocPolicy to mozilla::JSONWriter r=jwalden,mstange

Comment 8

[Noemi Erli[:noemi_erli]]

Backed out changeset bee1c344cb47 (Bug 1491909) for failures in browser/components/extensions/test/xpcshell/test_ext_geckoProfiler_control.js

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=bee1c344cb473d75347c012bbd0f8eac58428926&selectedJob=207401534

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=207401534&repo=autoland&lineNumber=1624

Backout: https://hg.mozilla.org/integration/autoland/rev/3b8a9abe2766c4bdc68c143416a3aafe85083be4

Comment 9

[Denis Palmeiro [:denispal]]

I can reproduce this error on Windows 10, but if I pull it up in WinDBG or Visual studio the test will pass so I'm unsure of how to collect a stack trace to see what is causing the assert.  It's definitely related to making the JSONWriter in the profiler use the JS::SystemAllocPolicy allocator, but I can't pinpoint where exactly the assert gets triggered.  I may need to think of another approach to all of this.

Comment 10

[Denis Palmeiro [:denispal]]

The error in comment 8 is due to the JS engine not being initialized.  This step also creates the malloc arena required to use js::SystemAllocPolicy.  Unfortunately, many of the threads that end up using JSONWriter in the profiler run in background threads, and there is a potential for a race condition if the main thread also tries to initialize the JS engine.  It is also undesirable to initialize the JS engine on the GPU and Network processes just to use the malloc arena.  Therefore, I will implement a deep copy of the event data instead as a first step between the TraceLogger and Profiler,  and if required, we can pursue a faster approach afterwards.

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:denispal, could you have a look please?

Comment 12

[Denis Palmeiro [:denispal]]

(In reply to Release mgmt bot [:sylvestre / :calixte] from comment #11)

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:denispal, could you have a look please?

In order to use the JS allocators for the profiler, we have to first initialize the JS engine on each process the profiler is sampling and this also includes the GPU process. We decided this wasn't a good idea to do on the GPU process, so we decided to go with a deep copy of the data instead and this patch is not required for that.