Closed Bug 1491909 Opened 1 year ago Closed 1 year ago
Allow the use of mozilla::JSONWriter in Spider
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!
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  contains definitions which are necessary to compile SpiderMonkey.  https://searchfox.org/mozilla-central/rev/6c82481caa506a240a626bb44a2b8cbe0eedb3a0/js/public/Utility.h#52-437 Apart from the custom allocator issue, SpiderMonkey also has this class , which may be sufficient for outputting JSON code.  https://searchfox.org/mozilla-central/source/js/src/vm/JSONPrinter.h
> Apart from the custom allocator issue, SpiderMonkey also has this class , > which may be sufficient for outputting JSON code. > >  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.
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.
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/bee1c344cb47 Add an AllocPolicy to mozilla::JSONWriter r=jwalden,mstange
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
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.
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.
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → INACTIVE
You need to log in before you can comment on or make changes to this bug.