Closed Bug 1491909 Opened 1 year ago Closed 1 year ago

Allow the use of mozilla::JSONWriter in SpiderMonkey

Categories

(Core :: JavaScript Engine, defect, P2)

defect

Tracking

()

RESOLVED INACTIVE
Tracking Status
firefox64 --- affected

People

(Reporter: denispal, Assigned: denispal)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
Hi Steve, here is the bug we discussed in #jsapi.  Thanks!
Flags: needinfo?(sfink)
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.
Flags: needinfo?(sfink) → needinfo?(jwalden+bmo)
(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
> 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.
Assignee: nobody → dpalmeiro
No longer blocks: 1480104
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.
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.
Flags: needinfo?(jwalden+bmo)
Blocks: 1497016
Priority: -- → P2
Blocks: 1501377
Keywords: checkin-needed
Pushed by aiakab@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bee1c344cb47
Add an AllocPolicy to mozilla::JSONWriter r=jwalden,mstange
Keywords: checkin-needed
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.
Flags: needinfo?(dpalmeiro)
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.

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?

Flags: needinfo?(dpalmeiro)

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

Status: NEW → RESOLVED
Closed: 1 year ago
Flags: needinfo?(dpalmeiro)
Resolution: --- → INACTIVE
You need to log in before you can comment on or make changes to this bug.