Closed Bug 1150253 Opened 9 years ago Closed 9 years ago

[jsdbg2] Post a runnable to fire the onGarbageCollection hook

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla40
Tracking Flags:
Tracking Status
firefox40 --- fixed

People

(Reporter: fitzgen, Assigned: fitzgen)

References

Details

Attachments

(3 files, 12 obsolete files)

Part 1: SpiderMonkey should call an embedder-provided callback instead of running the onGarbageCollection hook immediately
30.83 KB, patch
fitzgen
: review+
Details | Diff | Splinter Review
Part 2: Gecko should provide a callback for SpiderMonkey to enqueue the onGarbageCollection hook runnable
8.87 KB, patch
fitzgen
: review+
Details | Diff | Splinter Review
Part 3: Migrate onGarbageCollection tests
15.50 KB, patch
fitzgen
: review+
Details | Diff | Splinter Review 
There are various assertion failures because a lot of code expects the nursery to be empty after a GC, but that isn't currently the case because the Debugger's onGarbageCollection hook can run JS at the end of a GC. We should change that so that it just posts a runnable to gecko which then calls back into SpiderMonkey to fire the Debugger's hook.
Comment on attachment 8587074 [details] [diff] [review]
Part 1: SpiderMonkey should call an embedder-provided callback instead of running the onGarbageCollection hook immediately

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

::: js/public/Debug.h
@@ +272,5 @@
> +
> +class GarbageCollectionData;
> +
> +// TODO FITZGEN
> +typedef bool (*EnqueueOnGCRunnable)(JSRuntime* rt, UniquePtr<GarbageCollectionData>&& data);

The name seems weird. It's saying what the embedding is going to use it for. Shouldn't this be from the perspective of the engine, where it's a PostGCHook or HandleGCRecord or GCStatsCollectionHook or something?

@@ +280,5 @@
> +SetEnqueueOnGCRunnable(JSRuntime* rt, EnqueueOnGCRunnable enqueueOnGCRunnable);
> +
> +// TODO FITZGEN
> +JS_PUBLIC_API(bool)
> +FireOnGarbageCollectionHook(JSContext* cx, UniquePtr<GarbageCollectionData>&& data);

Why is this exposed publicly?

::: js/src/gc/Statistics.cpp
@@ +972,5 @@
> +
> +        if (runtime->debuggerEnqueueOnGCRunnable) {
> +            auto gcData = JS::dbg::GarbageCollectionData::Create(*this, runtime->gc.majorGCCount());
> +            if (!gcData || !runtime->debuggerEnqueueOnGCRunnable(runtime, mozilla::Move(gcData)))
> +                aborted = true;

I'm not sure how I feel about the 'aborted' flag being shared by internal stats generation failures as well as embedder failures. I think I'd rather those were split. (So eg you would still get the internal dumps even if you mistakenly set a GCStatsCollectionHook that always fails.) In fact, it makes more sense for these failures to be tracked by the embedding, not the engine, though I'm not 100% confident of that (given that the GCData::Create is most naturally detectable right here as you are doing it. The embedding would need to accept a nullptr and interpret it as an abort.)

::: js/src/vm/Debugger.h
@@ +969,5 @@
> + * The `GarbageCollectionData` class is essentially a view of the
> + * `js::gcstats::Statistics` data without the uber implementation-specific bits.
> + * It should generally be palatable for web developers.
> + */
> +class GarbageCollectionData {

GarbageCollectionRecord? GarbageCollectionEvent?
Attachment #8587074 - Flags: feedback?(sphink) → feedback+
Comment on attachment 8587075 [details] [diff] [review]
Part 2: Gecko should provide a callback for SpiderMonkey to enqueue the onGarbageCollection hook runnable

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

::: dom/base/nsJSEnvironment.cpp
@@ +2232,5 @@
> +  // TODO FITZGEN: should assert that we are on the originally provided
> +  // runtime's thread here.
> +
> +  AutoJSAPI jsapi;
> +  if (!JS::dbg::FireOnGarbageCollectionHook(jsapi.cx(), mozilla::Move(mGCData))) {

Oh! *This* is why you're exposing it publicly. Hm... if JS::dbg is meant for Debugger stuff, then I guess this is probably the right place and right name, but then setting the GC data collection hook should be in plain JS:: or js::.
Hm. In the process of converting the jit-tests for onGarbageCollection to xpcshell tests. Can't figure out how to create a global in any zone other than the system zone. Default is the system zone, however there is the `sameZoneAs` option, but then you need an object from another zone! Chicken and egg!
Depends on: 1153922
Comment on attachment 8591916 [details] [diff] [review]
Part 1: SpiderMonkey should call an embedder-provided callback instead of running the onGarbageCollection hook immediately

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

::: js/public/Debug.h
@@ +287,5 @@
> +// It should generally be palatable for web developers.
> +class GarbageCollectionEvent
> +{
> +    // The major GC number of the GC cycle this data pertains to.
> +    uint64_t gcNumber_;

Sorry for being picky, but we seem to have grown 3 types of "GC number": minorGCNumber, majorGCNumber, and plain gcNumber. That last is all we used to have, and most places assume that's what you mean by gcNumber. At this point, we probably ought to make distinct type names for all of them (perhaps just typedefs of uint64_t), but until then, I'd prefer if you called this majorGCNumber or gcMajorNumber everywhere, since it does not match the "normal" GC number and I'm afraid someone is going to end up doing if (event.gcNumber == rt->gc.gcNumber()).

Note that as far as I can tell, gcNumber will always be the same as minorGCNumber, though they're incremented in slightly different places. (Every major GC slice has to do a nursery GC, so it sort of makes sense.) But the way it's done kind of terrifies me. I'm going to ask Terrence about that.

@@ +298,5 @@
> +    // collection was forced to be non-incremental, this is a short reason of
> +    // why the GC could not perform an incremental collection.
> +    const char* nonincrementalReason;
> +
> +    // Represents a single garbage collection slice.

"Represents a single slice of a possibly multi-slice incremental garbage collection"?

@@ +332,5 @@
> +
> +// Provide SpiderMonkey with a function that notifies the embedder to call
> +// `JS::dbg::FireOnGarbageCollectionHook` "soon, but not now".
> +JS_PUBLIC_API(void)
> +SetEnqueueOnGCRunnable(JSRuntime* rt, JS::EnqueueOnGCRunnable enqueueOnGCRunnable);

I still think this is leaking embedder semantics. I would rather these be

typedef void (*GCEventHandler)(JSRuntime* rt, UniquePtr<dbg::GarbageCollectionEvent>&& data);
SetGCEventHandler(...);

Or PostGCEventHandler. Or something. Anything that doesn't say what you're going to do with it.

::: js/src/gc/Statistics.cpp
@@ +974,5 @@
> +            auto gcData = JS::dbg::GarbageCollectionEvent::Create(*this, runtime->gc.majorGCCount());
> +            if (gcData)
> +                runtime->debuggerEnqueueOnGCRunnable(runtime, mozilla::Move(gcData));
> +            else
> +                aborted = true;

I don't think this should set 'aborted'. If you need to use the flag for your own purposes, create a new flag. As it is, this is a no-op since aborted will be set back to false a few lines down.

As for what to do if the gcData creation fails, I'm not sure. We don't have a MOZ_WARN or anything that I can see. I'm ok if you just ignore it. The more pure solution would be to just pass the null in and say the embedding is supposed to accept null (or register an error callback for this purpose.)

If you're doing a DEBUG-only bare printf over in Zone.cpp, you may as well do one here too?

::: js/src/gc/Zone.cpp
@@ +270,5 @@
> +            if (!r.front()->debuggeeIsBeingCollected(rt->gc.majorGCCount())) {
> +#ifdef DEBUG
> +                printf("OOM while notifying observing Debuggers of a GC: The onGarbageCollection\n"
> +                       "hook will not be fired for this GC for some Debuggers!\n");
> +#endif

I don't think a bare printf is kosher. I have no idea what it'll do eg on b2g or android. But I could be wrong; I don't really know for sure. If it compiles everywhere, I have no fundamental objection given that it's DEBUG only.

::: js/src/vm/Debugger.cpp
@@ +7817,5 @@
> +    for (auto range = stats.sliceRange(); !range.empty(); range.popFront()) {
> +        if (!data->reason) {
> +            // XXX: There is only one GC reason for the whole cycle, but for
> +            // legacy reasons this data is stored and replicated on each
> +            // slice.

I would get rid of the "XXX: ". And would it be possible to explain the legacy reasons?

For the record, I'm not entirely happy with losing the per-slice reasons. But then, I don't know the full story; perhaps they weren't really valid. Anyway, if you wanted to claim that you're leaving it open for per-slice reasons to be added (back) in the future, I'd be fine with that.

@@ +7836,5 @@
> +
> +static bool
> +DefineStringProperty(JSContext* cx, HandleObject obj, PropertyName* propName, const char* strVal)
> +{
> +    RootedValue val(cx, NullValue());

Should this be UndefinedValue()? It seems like JS spec people, at least, are using "undefined" for "omitted or not meaningful" and null for the explicit absence of an object. Or something like that.

@@ +7884,5 @@
> +    RootedValue slicesValue(cx, ObjectValue(*slicesArray));
> +    if (!DefineProperty(cx, obj, cx->names().collections, slicesValue))
> +        return nullptr;
> +
> +    return obj.get();

A plain |return obj| will do.
Attachment #8591916 - Flags: review?(sphink) → review+
Comment on attachment 8591919 [details] [diff] [review]
Part 2: Gecko should provide a callback for SpiderMonkey to enqueue the onGarbageCollection hook runnable

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

As far as I can tell, you don't need to register the runnable in three different places, just once per JS runtime.  Is this supposed to cover the main thread and workers?  If so, you just need the code in CycleCollectedJSRuntime.  If it is mainthread only, you just need the code in XPCJSRuntime.

If you end up changing it so that the only use of the runnable is in CycleCollectedJSRuntime.cpp, you should move the new files you define into xpcom/.

::: dom/base/DebuggerOnGCRunnable.cpp
@@ +13,5 @@
> +/* static */ void
> +DebuggerOnGCRunnable::Enqueue(JSRuntime* aRuntime,
> +                              UniquePtr<JS::dbg::GarbageCollectionEvent>&& aGCData)
> +{
> +  // Make sure we are getting the right runtime for this thread.

This comment and the same one below seem superfluous.

::: dom/base/DebuggerOnGCRunnable.h
@@ +4,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef __mozilla_dom_DebuggerOnGCRunnable_h_
> +#define __mozilla_dom_DebuggerOnGCRunnable_h_

nit: No _ at the start or end of this symbol please.  See the include guard section here: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style

@@ +8,5 @@
> +#define __mozilla_dom_DebuggerOnGCRunnable_h_
> +
> +#include "nsThreadUtils.h"
> +#include "js/Debug.h"
> +#include "mozilla/dom/ScriptSettings.h"

nit: I think this include should go in the .cpp file.

@@ +16,5 @@
> +
> +namespace mozilla {
> +namespace dom {
> +
> +// Runnable to fire SpiderMonkey's Debugger API's onGarbageCollection hook.

nit: This phrasing is awkward to me.  Maybe "Runnable to fire the SpiderMonkey Debugger API's onGarbageCollection hook."?

@@ +19,5 @@
> +
> +// Runnable to fire SpiderMonkey's Debugger API's onGarbageCollection hook.
> +class DebuggerOnGCRunnable : public nsRunnable
> +{
> +  DebugOnly<JSRuntime*> mRuntime;

nit: Maybe put the debug-only thing second?  Seems a little weird for the first field of an opt version of this to be this weird void thing.  Maybe that's ridiculous.
Attachment #8591919 - Flags: review?(continuation) → review-
Comment on attachment 8591921 [details] [diff] [review]
Part 3: Migrate onGarbageCollection tests

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

I didn't look that closely at the tests, but they seem generally sane.

::: js/src/builtin/TestingFunctions.cpp
@@ +225,5 @@
>              if (!JS_StringEqualsAscii(cx, arg.toString(), "compartment", &compartment))
>                  return false;
>          } else if (arg.isObject()) {
> +            for (ZonesIter z(cx->runtime(), WithAtoms); !z.done(); z.next())
> +                JS::SkipZoneForGC(z.get());

So, this seems reasonable yet dangerous. Nobody seems to know for sure whether it is safe to unschedule zones. And yet, this function is only for testing, so we're ok letting the try server decide.
Attachment #8591921 - Flags: review?(sphink) → review+

        Attachment #8591921 -
        Attachment is obsolete: true

        Attachment #8594373 -
        Flags: review+

    
    

    
  
Comment on attachment 8594372 [details] [diff] [review]
Part 2: Gecko should provide a callback for SpiderMonkey to enqueue the onGarbageCollection hook runnable

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

::: dom/base/nsJSEnvironment.cpp
@@ +44,5 @@
>  #include "xpcpublic.h"
>  
>  #include "jsapi.h"
>  #include "jswrapper.h"
> +#include "js/Debug.h"

Remove this include.

::: xpcom/base/CycleCollectedJSRuntime.cpp
@@ +771,5 @@
> +    NS_WARN_IF(NS_FAILED(DebuggerOnGCRunnable::Enqueue(aRuntime, aDesc)));
> +  }
> +
> +  if (self->mPrevGCSliceCallback) {
> +    (self->mPrevGCSliceCallback(aRuntime, aProgress, aDesc));

nit: no outer parens here.

::: xpcom/base/DebuggerOnGCRunnable.cpp
@@ +4,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "mozilla/CycleCollectedJSRuntime.h"
> +#include "mozilla/DebuggerOnGCRunnable.h"

nit: put the DebuggerOnGCRunnable.h header first, to ensure that it doesn't implicitly depend on other files. (Well, I guess with unified builds that isn't necessarily true, but still.)

@@ +14,5 @@
> +/* static */ NS_METHOD
> +DebuggerOnGCRunnable::Enqueue(JSRuntime* aRt, const JS::GCDescription& aDesc)
> +{
> +  auto gcEvent = aDesc.toGCEvent(aRt);
> +  if (!gcEvent)

nit: brace the body of the if.

::: xpcom/base/DebuggerOnGCRunnable.h
@@ +8,5 @@
> +#define mozilla_DebuggerOnGCRunnable_h
> +
> +#include "nsThreadUtils.h"
> +#include "js/GCAPI.h"
> +#include "mozilla/dom/ScriptSettings.h"

Please move the include of ScriptSettings.h into the .cpp.

@@ +9,5 @@
> +
> +#include "nsThreadUtils.h"
> +#include "js/GCAPI.h"
> +#include "mozilla/dom/ScriptSettings.h"
> +#include "mozilla/DebugOnly.h"

Please remove the include of DebugOnly, which doesn't seem to be used any more.

        Attachment #8594372 -
        Flags: review?(continuation) → review+

    
    

    
  


  

        Attachment #8594373 -
        Attachment is obsolete: true

        Attachment #8595009 -
        Flags: review+

    
    

    
  
Carrying over r+.

Changes:

* Don't unschedule zones for gc in the testing function, just set the JSGC_MODE_WHATEVER.

* Use JS::DeletePolicy with the UniquePtr<GarbageCollectionEvent>

* Fix shadowing warning that was causing builds to fail

* Only call gczeal in xpcshell tests if it is available (not available in opt builds, I think)

New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3562272b8caf

    
    

    
  
Carrying over r+ again.

Changes:

* Use JSRuntime::make_unique to create the GarbageCollectionEvent UniquePtrs so that check_vanilla_allocations.py is happy. This involved typing out a TON of `mozilla::UniquePtr<JS::dbg::GarbageCollectionEvent, JS::DeletePolicy<JS::dbg::GarbageCollectionEvent>>` everywhere, so I just aliased that to `JS::dbg::GarbageCollectionEvent::Ptr` for sanity and line length concerns.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1accc56ff1e4

    
    

    
  
(In reply to Nick Fitzgerald [:fitzgen] from comment #24)
> This involved typing out a
> TON of `mozilla::UniquePtr<JS::dbg::GarbageCollectionEvent,
> JS::DeletePolicy<JS::dbg::GarbageCollectionEvent>>` everywhere, so I just
> aliased that to `JS::dbg::GarbageCollectionEvent::Ptr` for sanity and line
> length concerns.

Note that bug 1156562 will help with this in the future.

    
    

    
  
Ok, so it looks like the jit-tests aren't happy about messing with the JSGC_MODE_WHATEVER in the gc() testing function. Exploring workarounds.

    
    

    
  
(In reply to Nick Fitzgerald [:fitzgen] from comment #27)
> Created attachment 8595491 [details] [diff] [review]
> Part 3: Migrate onGarbageCollection tests

As discussed with sfink on irc, we are loosening the assertions around other zones _not_ GC'ing.

New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a76b9cd28cb

    
    

    
  
Looks good!
Keywords: checkin-needed

    
    

    
  
part 1 seems to have problems to apply:

renamed 1150253 -> Bug-1150253---Part-1-SpiderMonkey-should-call-an-e.patch
applying Bug-1150253---Part-1-SpiderMonkey-should-call-an-e.patch
patching file js/src/vm/Debugger.h
Hunk #3 FAILED at 240
Hunk #5 FAILED at 666
Hunk #7 FAILED at 975
3 out of 7 hunks FAILED -- saving rejects to file js/src/vm/Debugger.h.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh Bug-1150253---Part-1-SpiderMonkey-should-call-an-e.patch

could you take a look, thanks!
Flags: needinfo?(nfitzgerald)
Keywords: checkin-needed
Depends on: 1379957

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: