Closed Bug 1065657 Opened 5 years ago Closed 5 years ago

[jsdbg2] Allow more than one Debugger instance to track allocation sites at a time

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: fitzgen, Assigned: fitzgen)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 5 obsolete files)

When we first implemented allocation site tracking, we restricted it to one Debugger instance at a time for simplicity of implementation. We should go back and make it properly work with N Debuggers.
I think it wouldn't be too hard to do this. Before we call JSCompartment::forgetObjectMetadataCallback, we could iterate over its global's DebuggerVector, and see if there exists any other Debugger that is tracking allocation sites. In other words, recompute the "reference count" each time.
That sounds pretty simple :P

We would also need to check that the existing metadata callback is the SavedStacksMetadataCallback, if one already exists, when setting trackingAllocationSites and when adding new debuggees.
Assignee: nobody → nfitzgerald
Status: NEW → ASSIGNED
Comment on attachment 8592469 [details] [diff] [review]
Allow multiple Debuggers to track allocations at the same time

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

What about setting .enabled?

Generally looks pretty good to me. Would like answers to the question above and the getAllocationSite question below before r+.

::: js/src/vm/Debugger.cpp
@@ +3235,5 @@
> +/* static */ bool
> +Debugger::CannotTrackAllocations(const GlobalObject& global)
> +{
> +    auto existingCallback = global.compartment()->getObjectMetadataCallback();
> +    return existingCallback && existingCallback != SavedStacksMetadataCallback;

No need to check for non-nullness of existingCallback.

@@ +3281,5 @@
> +                anyDebuggersTrackingAllocations = true;
> +                break;
> +            }
> +        }
> +    }

Given that checking whether there's a Debugger instance that's tracking allocations is used both in DEBUG and non-DEBUG, could you factor this out to a private method and use that both here and in AddAllocationsTracking?

@@ +6818,5 @@
> +        return true;
> +    }
> +
> + noMetadata:
> +    args.rval().setNull();

What is the change to this function supposed to be doing?

::: js/src/vm/Debugger.h
@@ +326,5 @@
> +     * Remove allocations tracking for objects allocated within the given
> +     * global's compartment. This is a no-op if there are still Debuggers
> +     * observing this global and who are tracking allocations.
> +     */
> +    static void RemoveAllocationsTracking(GlobalObject& global);

Style nit: we aren't full Gecko style yet, let's lowercase these methods. ;)

::: js/src/vm/DebuggerMemory.cpp
@@ +157,5 @@
>      for (WeakGlobalObjectSet::Range r = dbg->debuggees.all(); !r.empty(); r.popFront()) {
>          if (enabling) {
> +            // Should always succeed, since we already checked for the error
> +            // case above.
> +            MOZ_ASSERT(Debugger::AddAllocationsTracking(cx, *r.front().get()));

Use MOZ_ALWAYS_TRUE. MOZ_ASSERT(expr) is going to be elided completely in non-DEBUG builds, whereas MOZ_ALWAYS_TRUE(expr) will always evaluate expr, but only check the rv in DEBUG builds.

::: js/src/vm/SavedStacks.cpp
@@ +1172,5 @@
>          MOZ_ASSERT(dbgs->begin() == begin);
>  
>          if ((*dbgp)->trackingAllocationSites && (*dbgp)->enabled)
> +            allocationSamplingProbability = std::max((*dbgp)->allocationSamplingProbability,
> +                                                     allocationSamplingProbability);

Could you update the docs to say that the probability is max'd with all the other Debuggers on a global?

Style nit: { } around the if here
(In reply to Shu-yu Guo [:shu] from comment #4)
> @@ +6818,5 @@
> > +        return true;
> > +    }
> > +
> > + noMetadata:
> > +    args.rval().setNull();
> 
> What is the change to this function supposed to be doing?

This is to avoid returning any old random metadata, and only return it if the metadata is a SavedFrame object. The test changes exposed this existing bug.

> ::: js/src/vm/SavedStacks.cpp
> @@ +1172,5 @@
> >          MOZ_ASSERT(dbgs->begin() == begin);
> >  
> >          if ((*dbgp)->trackingAllocationSites && (*dbgp)->enabled)
> > +            allocationSamplingProbability = std::max((*dbgp)->allocationSamplingProbability,
> > +                                                     allocationSamplingProbability);
> 
> Could you update the docs to say that the probability is max'd with all the
> other Debuggers on a global?

Good call.
(In reply to Shu-yu Guo [:shu] from comment #4)
> ::: js/src/vm/Debugger.cpp
> @@ +3235,5 @@
> > +/* static */ bool
> > +Debugger::CannotTrackAllocations(const GlobalObject& global)
> > +{
> > +    auto existingCallback = global.compartment()->getObjectMetadataCallback();
> > +    return existingCallback && existingCallback != SavedStacksMetadataCallback;
> 
> No need to check for non-nullness of existingCallback.

We do, because if there is no existing callback, then we *can* track allocations. If we didn't have that check, then this function would return true (that we can *not* track allocations) because nullptr != SavedStacksMetadata.
Attachment #8592469 - Attachment is obsolete: true
Attachment #8592469 - Flags: review?(shu)
Attachment #8594218 - Flags: review?(shu)
(In reply to Nick Fitzgerald [:fitzgen] from comment #6)
> (In reply to Shu-yu Guo [:shu] from comment #4)
> > ::: js/src/vm/Debugger.cpp
> > @@ +3235,5 @@
> > > +/* static */ bool
> > > +Debugger::CannotTrackAllocations(const GlobalObject& global)
> > > +{
> > > +    auto existingCallback = global.compartment()->getObjectMetadataCallback();
> > > +    return existingCallback && existingCallback != SavedStacksMetadataCallback;
> > 
> > No need to check for non-nullness of existingCallback.
> 
> We do, because if there is no existing callback, then we *can* track
> allocations. If we didn't have that check, then this function would return
> true (that we can *not* track allocations) because nullptr !=
> SavedStacksMetadata.

Ah okay, right.
Comment on attachment 8594218 [details] [diff] [review]
Allow multiple Debuggers to track allocations at the same time

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

LGTM.

::: js/src/doc/Debugger/Debugger.Memory.md
@@ +33,5 @@
>  
> +- A [Bernoulli trial][bernoulli-trial] succeeds, with probability equal to the
> +  maximum of
> +  [`d.memory.allocationSamplingProbability`][alloc-sampling-probability] for
> +  each `Debugger` instance `d` that is observing the global that this object is

The "for each" wording doesn't sound obvious to me.

for each [...] that is observing -> of all [...] that are observing

::: js/src/vm/Debugger.cpp
@@ +6846,5 @@
>      THIS_DEBUGOBJECT_REFERENT(cx, argc, vp, "get allocationSite", args, obj);
>  
>      RootedObject metadata(cx, GetObjectMetadata(obj));
> +    if (!metadata)
> +        goto noMetadata;

I don't think there's enough code shared to warrant goto here. I don't feel that strongly though, it's perfectly readable.

::: js/src/vm/DebuggerMemory.cpp
@@ +131,5 @@
>      Debugger* dbg = memory->getDebugger();
>      bool enabling = ToBoolean(args[0]);
>  
> +    if (enabling == dbg->trackingAllocationSites)
> +        goto done;

Ditto as above comment on goto.
Attachment #8594218 - Flags: review?(shu) → review+
(In reply to Nick Fitzgerald [:fitzgen] from comment #11)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=61f8808c8164

seems this caused a bustage according to the try run or ?
Flags: needinfo?(nfitzgerald)
Keywords: checkin-needed
Unused variable. Will fix it up in a minute.
Flags: needinfo?(nfitzgerald)
New try push is green!
Keywords: checkin-needed
Hi, this does not apply:

Parsing... done
adding 1065657 to series file
renamed 1065657 -> Bug-1065657---Allow-multiple-Debuggers-to-track-al.patch
applying Bug-1065657---Allow-multiple-Debuggers-to-track-al.patch
patching file js/src/vm/Debugger.cpp
Hunk #1 FAILED at 1721
1 out of 6 hunks FAILED -- saving rejects to file js/src/vm/Debugger.cpp.rej
patching file js/src/vm/Debugger.h
Hunk #1 FAILED at 303
1 out of 1 hunks FAILED -- saving rejects to file js/src/vm/Debugger.h.rej
patching file js/src/vm/SavedStacks.cpp
Hunk #2 FAILED at 1156
1 out of 2 hunks FAILED -- saving rejects to file js/src/vm/SavedStacks.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh Bug-1065657---Allow-multiple-Debuggers-to-track-al.patch

could you take a look, thanks!
Flags: needinfo?(nfitzgerald)
And of course inbound is perpetually closed :(
Flags: needinfo?(nfitzgerald)
Keywords: checkin-needed
Follow up to remove bad friend declaration that caused GCC to warn and burn inbound.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/68fe0894bb09
https://hg.mozilla.org/mozilla-central/rev/532169437c2f
https://hg.mozilla.org/mozilla-central/rev/68fe0894bb09
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
sorry had to backout this changes in https://hg.mozilla.org/mozilla-central/rev/0b202671c9e2 since one of this changes seems to have caused a Linux PGO Bustage like:

https://treeherder.mozilla.org/logviewer.html#?job_id=1380632&repo=mozilla-central
Status: RESOLVED → REOPENED
Flags: needinfo?(nfitzgerald)
Resolution: FIXED → ---
Attachment #8595449 - Attachment is obsolete: true
Attachment #8596050 - Attachment is obsolete: true
Attachment #8596660 - Flags: review+
Looks like xpcshell on PGO is fine here? Failures seem to be builds that don't support PGO.
Keywords: checkin-needed
(In reply to Nick Fitzgerald [:fitzgen] from comment #26)
> Looks like xpcshell on PGO is fine here? Failures seem to be builds that
> don't support PGO.

The Bustage we had yesterday was on Linux Pgo i think, do you know why there is no linux pgo on this run ? (seems you did all the configs needed for pgo but somehow i think linux is missing there)
Flags: needinfo?(nfitzgerald)
Keywords: checkin-needed
(In reply to Carsten Book [:Tomcat] from comment #27)
> (In reply to Nick Fitzgerald [:fitzgen] from comment #26)
> > Looks like xpcshell on PGO is fine here? Failures seem to be builds that
> > don't support PGO.
> 
> The Bustage we had yesterday was on Linux Pgo i think, do you know why there
> is no linux pgo on this run ? (seems you did all the configs needed for pgo
> but somehow i think linux is missing there)

RyanVM assured me that my changes to mozconfig.common.overrides enabled PGO on linux, but that the try pushes don't get the nicely labeled names that you would get pushing to inbound.

See also https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c5b44163119 from bug 1024774, which breaks xpcshell on linux PGO, and so I am 99% sure that was the offender.
Flags: needinfo?(nfitzgerald)
Re-adding checkin-needed per comment 28.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/07900c19ad29
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.