1068988 - [jsdbg2] Add byte size of each allocation to allocations log

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Almost all Object allocations are equal (since size grows as you add more properties and really lives in Shape/BaseShape). However, there is one case that bucks that trend and it would be very valuable to know allocation size since it could take up a ton of space: typed arrays.

After bug 1066313 and this bug, the entries in the allocations log would be of the form { frame, timestamp, size }.

Comment 1

[Nicholas Nethercote [inactive]]

> Almost all Object allocations are equal (since size grows as you add more
> properties and really lives in Shape/BaseShape). However, there is one case
> that bucks that trend and it would be very valuable to know allocation size
> since it could take up a ton of space: typed arrays.

That's not quite right.

First, there are six different sizes of JSObject GC things, having 0, 2, 4, 8, 12 or 16 slots, where slots hold properties and a few other things. See http://mxr.mozilla.org/mozilla-central/source/js/src/gc/Heap.h#72).

Second, JSObjects can have dynamic slots (for properties, mostly) and dynamic elements, both of which are allocated on the malloc heap. For typed arrays I think the dynamic elements hold the typed array elements, but I'm not certain. But slots and elements can be large even for objects that aren't typed arrays, and so shouldn't be ignored.

Third, JSObjects of some classes can have other malloc'd bits and pieces hanging off them.

Look at JSObject::addSizeOfExcludingThis() to see all this in action. If you're measuring object memory usage, you should use that function.

As for shapes, they're auxiliary structures that describe how properties are laid out in objects, but they don't actually hold the property values. Those are stored in the object slots.

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: WIP: Add byte size to the allocation log

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

One concern, it seems gecko doesn't ever call `JS::dbg::SetDebuggerMallocSizeOf` and that is only done in the js shell.

That will need to happen (probably in CycleCollectedJSRuntime?) for this to be useful.

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Working on top of bug 1134865, which messes a bit with the allocations log as well.

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Jim, do you think we need to start sampling per-byte with this change, or can we push that off as a follow up? That will require much more invasive changes to the allocations log than just adding the size of allocations we sample now.

Comment 6

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

See comment 5

Comment 7

[Jim Blandy :jimb]

Yeah, this means that the likelihood of an object appearing in the log depends on how many times it gets expanded, and that's pretty meaningless. Ignoring expansions defeats the purpose. Switching to sampling per byte would be a good thing.

Perhaps it's obvious, but we should be careful to sample on the *increase* in size, not on the *new* size. I'm not sure how you're going to get that.

Speaking of sampling: are consumers of the allocation log prepared to hear about the expansion of objects whose initial creation they never saw?

Comment 8

[Jim Blandy :jimb]

Oh, I'm sorry, I misread the purpose of this patch.

You're saying simply that we want to add allocation size to log entries for the *initial allocation*, which is meaningful in a few cases. You're *not* tracking object expansion in this patch.

In that case, I think it's still okay (well, as okay as it was before) to sample object creations, not byte allocations, as long as the displays are clear that we're displaying a representative sample of objects, not bytes.

Comment 9

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Hrm, we fire the allocation hook before native objects' reserved slots are initialized and this is making the code for getting the size of TypedArrayObjects and ArrayBufferObjects fail assertions. Even if I fix the assertions, we will get the size of these things without the interesting internal state.

What if we had an optional flag/type for passing into NewObject that said "I promise to call the allocations hook after I initialize the reserved slots"? Maybe an RAII that asserts on destruction if you didn't call the metadata hook via one of its methods and the cx doesn't have a pending exception?

Comment 10

[Jim Blandy :jimb]

I'm not at all familiar with how the allocation log is connected to object allocation. Clearly we'll need to wait until the object is fully initialized for ubi::Node::size's sake before we report it, but if we're logging from the object metadata callback, that's definitely too early; that's called before we've even set the object's shape. We'll have to split out the logging from the object metadata callback.


This reminds me a bit of the onNewGlobalObject hook. One problem we ran into there was that allocating the JS object was only the beginning of the process; DOM globals have all kinds of other gunk attached to them which their accessors expect to be present. We called the onNewGlobalObject hooks as soon as the JSObject itself was ready; but then that hook could try to do all kinds of things to the global that it wasn't prepared for. In a sense, it wasn't fully initialized; to draw an analogy with C++ construction, the JSObject creation was really just a base class constructor, and Gecko had a series of more derived class constructors to run yet, at the point we were reporting the global to the Debugger.

In the end, we put a flag on the global indicating whether we'd reported it via onNewGlobalObject yet, and then asserted that that flag had been set before we ever ran JavaScript in its scope. Or something like that.

That's not directly relevant, but I thought I'd repeat the story in case it points out further concerns we haven't dealt with.

Comment 11

[Shu-yu Guo [:shu]]

(In reply to Nick Fitzgerald [:fitzgen] from comment #9)
> Hrm, we fire the allocation hook before native objects' reserved slots are
> initialized and this is making the code for getting the size of
> TypedArrayObjects and ArrayBufferObjects fail assertions. Even if I fix the
> assertions, we will get the size of these things without the interesting
> internal state.
> 
> What if we had an optional flag/type for passing into NewObject that said "I
> promise to call the allocations hook after I initialize the reserved slots"?
> Maybe an RAII that asserts on destruction if you didn't call the metadata
> hook via one of its methods and the cx doesn't have a pending exception?

How do you envisioning this being implemented to ensure such a hook is called after all the reserved slots are initialized? I'm not aware of a unified path off the top of my head.

But suppose there were, would this be the only hook that needs to be called? That is, would the metadata hook still be needed?

Comment 12

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Jim, since bhacket made metadata stored in a weakmap rather than the base shape, the metadata callback gets a JSObject with its shape and object group all initialized. The reserved slots are all still undefined, though.

I was thinking something like this:

  class AutoDelayMetadataCallback {
      JSContext* cx;
      bool called;

    public:
      AutoDelayMetadataCallback(JSContext* cx)
          : cx(cx),
            called(false)
      { }

      void callMetadataCallback(JSObject* obj) {
          called = true;
          if (cx->compartment()->hasObjectMetadataCallback()) {
              SetNewObjectMetadata(cx, obj);
          }
      }

      ~AutoDelayMetadataCallback() {
          // Ensure that the metadata callback was called for this object.
          MOZ_ASSERT_IF(!cx->isExceptionPending(), called);
      }
  };

JSObject::create would take a `Maybe<AutoDelayMetadataHook> maybeDelayMetadata` parameter and then instead of always setting metadata, do the following:

  if (maybeDelayMetadata.isNone())
      SetNewObjectMetadata(cx, obj);
  // else: The AutoDelayMetadataCallback will handle calling the metadata callback
  // later, after it initializes reserved slots.

This approach wouldn't ensure that there is one common path we always go through after initializing reserved slots, but then if you opt out of calling the metadata hook immediately then you are held to your promise that you will call it later. Additionally, in practice, I've found that sizeOf{Including,Excluding}This style methods for various NativeObjects will fail assertions if you call them on objects whose reserved slots haven't been initialized, so that would nudge people towards using AutoDelayMetadataCallback.

Comment 13

[Jim Blandy :jimb]

(In reply to On vacation until May 17th from comment #12)
> This approach wouldn't ensure that there is one common path we always go
> through after initializing reserved slots, but then if you opt out of
> calling the metadata hook immediately then you are held to your promise that
> you will call it later. Additionally, in practice, I've found that
> sizeOf{Including,Excluding}This style methods for various NativeObjects will
> fail assertions if you call them on objects whose reserved slots haven't
> been initialized, so that would nudge people towards using
> AutoDelayMetadataCallback.

This seems very sound.

However, if you look through who calls JSObject::create, you'll find a lot of generic object allocation functions like js::NewObjectWithGivenTaggedProto, and then if you look for their callers, ... there are a lot of sites that allocate objects. A better heuristic would be to look for uses of JSCLASS_HAS_RESERVED_SLOTS; there are 73 under js/src, and every site at which one of those classes is instantiated would need to be examined and modified. So I wonder how feasible it is; and how people will feel about these extra arguments being so prominent.

Looking at JSObject::addSizeOfExcludingThis:
https://hg.mozilla.org/mozilla-central/file/6b9fcd51adc1/js/src/jsobj.cpp#l4005

there are actually very few classes that need delayed handling, because addSizeOfExcludingThis only cares about the object classes that actually consume a lot of memory in practice.

Perhaps it would be workable to have a JSCLASS_ flag saying, "please wait to log my allocation"; and then a field in the JSCompartment holding the sole object whose allocation we have not yet reported. GC and various hot spots could assert that that field was empty. Trying to store a new object in the slot while it was full would assert. And the delayed logging would clear the slot.

This certainly doesn't have the nice statically-checked, guaranteed error character of your solution, which is a good thing. The change just seems so invasive, I'm uncomfortable with that.

Comment 14

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Jim Blandy :jimb from comment #13)

The other benefit this has, besides being less invasive, is that it enforces the ordering of the log (at least within compartments).

Will take a crack at it!

Comment 15

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 1: Add byte size to the allocation log

Comment 16

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 2: Add a test for byte sizes in the allocations log

Comment 17

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 3: Document the size property in the allocations log

Comment 18

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 1: Add byte size to the allocation log

Comment 19

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #18)
> Created attachment 8610916 [details] [diff] [review]
> Part 1: Add byte size to the allocation log

I had forgot to document some stuff, and call sweepObjectPendingMetadata. Fixed now.

Comment 20

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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

Comment 21

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Well that was a rather unhappy try push. Unfortunately, I don't know if it is practical to maintain the invariant that the metadata callback is called in order (ie, that we have no more than one object pending metadata at a time). I was able to run the jit tests locally and not trigger the invariant's assertions, but it sure did trigger a lot on try,

Comment 22

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 1: Add byte size to the allocation log

Comment 23

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Ok that was actually pretty easy to fix.

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

Comment 24

[Shu-yu Guo [:shu]]

Comment on attachment 8611452 [details] [diff] [review]
Part 1: Add byte size to the allocation log

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

Since this API is pretty ad-hoc, we hashed out an improvement on IRC to
tighten the assertions. I think we want to maintain the following invariants:

- RAII inside Class constructors to prevent forgetting to discharge pending
  new objects with metadata.

- Allocation paths themselves should assert that their caller in fact promises
  to discharge pending new objects that get saved on JSCompartment, if the
  metadata stuff is to be delayed.

The idea is this:

Have the delayNewObjectMetadata flag on Class, as you do in this patch.

Have a field on JSCompartment, like pendingNewObjectWithMetadata, as you do in
this patch.

Have an RAII class AutoSetNewObjectMetadata for use inside the "constructors"
of Classes that shouldDelayNewObjectMetadata.

The constructor sets compartment->pendingNewObjectWithMetadata to a sentinel
"I'm expecting a JSObject*". (1)

The destructor asserts that compartment->pendingNewObjectWithMetadata is
null-or-JSObject* (i.e., not the sentinel value). If non-null, call
SetNewObjectMetadata.

Inside the allocation paths, if allocating with a Class that
shouldDelayNewObjectMetadata, assert that compartment->pendingNewObject is the
sentinel value from (1). Assign the allocated pointer (or nullptr on failure)
to compartment->pendingNewObject.

Otherwise, assert that compartment->pendingNewObject is null and call
SetNewObjectMetadata directly.

::: js/src/jscompartment.cpp
@@ +559,5 @@
> +    if (objectPendingMetadata_.unbarrieredGet())
> +        // We should never finalize an object before it gets its metadata! That
> +        // would mean we aren't calling the object metadata callback for every
> +        // object!
> +        MOZ_ALWAYS_TRUE(!IsAboutToBeFinalized(&objectPendingMetadata_));

Style nit: { } around the if

Did you intend this to be a release assert? In that case use MOZ_RELEASE_ASSERT.

::: js/src/jscompartment.h
@@ +302,5 @@
> +        // objects slip through the cracks: assert that the previously pending
> +        // object had its metadata set before we enqueue the next one.
> +        MOZ_ASSERT(objectPendingMetadata_ == nullptr);
> +        objectPendingMetadata_.set(obj);
> +    }

I don't think this method should be called directly, since currently this is setting objectPendingMetadata_ even when off-thread with a non-JSContext ExclusiveContext.

Best to have a SetPendingNewObjectMetaData helper like SetNewObjectMetadata that is a nop when given a non-JSContext.

::: js/src/jsobjinlines.h
@@ +317,5 @@
> +    if (group->clasp()->shouldDelayMetadataCallback()) {
> +        cx->compartment()->setObjectPendingMetadata(obj);
> +    } else {
> +        SetNewObjectMetadata(cx, obj);
> +    }

Style nit: SM doesn't brace 1-line conditional bodies yet.

::: js/src/vm/ArrayBufferObject.cpp
@@ +827,5 @@
>      } else {
>          obj->initialize(nbytes, contents, ownsState);
>      }
>  
> +    SetNewObjectMetadata(cx, obj);

Was this just missing a call to SetNewObjectMetadata before?

::: js/src/vm/ArrayObject-inl.h
@@ +69,5 @@
> +    if (obj->getClass()->shouldDelayMetadataCallback()) {
> +        cx->compartment()->setObjectPendingMetadata(obj);
> +    } else {
> +        SetNewObjectMetadata(cx, obj);
> +    }

Isn't obj here always of class ArrayObject::class_? Seems like the shouldDelayMetadataCallback check is not needed.

Comment 25

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

New patch up in a second.

(In reply to Shu-yu Guo [:shu] from comment #24)
> ::: js/src/jscompartment.cpp
> @@ +559,5 @@
> > +    if (objectPendingMetadata_.unbarrieredGet())
> > +        // We should never finalize an object before it gets its metadata! That
> > +        // would mean we aren't calling the object metadata callback for every
> > +        // object!
> > +        MOZ_ALWAYS_TRUE(!IsAboutToBeFinalized(&objectPendingMetadata_));
> 
> Style nit: { } around the if

So if there are comments for the single consequent statement we should wrap with braces?

> Did you intend this to be a release assert? In that case use
> MOZ_RELEASE_ASSERT.

No, it should always be evaluated so that if the object is moved, this reference is updated. However, I think it only needs to cause an assertion failure in debug builds. Maybe my intuition of when something should be a release assert is just off, though?

> ::: js/src/jscompartment.h
> @@ +302,5 @@
> > +        // objects slip through the cracks: assert that the previously pending
> > +        // object had its metadata set before we enqueue the next one.
> > +        MOZ_ASSERT(objectPendingMetadata_ == nullptr);
> > +        objectPendingMetadata_.set(obj);
> > +    }
> 
> I don't think this method should be called directly, since currently this is
> setting objectPendingMetadata_ even when off-thread with a non-JSContext
> ExclusiveContext.

Fixed in new version.

> ::: js/src/vm/ArrayBufferObject.cpp
> @@ +827,5 @@
> >      } else {
> >          obj->initialize(nbytes, contents, ownsState);
> >      }
> >  
> > +    SetNewObjectMetadata(cx, obj);
> 
> Was this just missing a call to SetNewObjectMetadata before?

This was because ArrayBufferObject's JSClass is now JSCLASS_DELAY_OBJECT_METADATA.

> ::: js/src/vm/ArrayObject-inl.h
> @@ +69,5 @@
> > +    if (obj->getClass()->shouldDelayMetadataCallback()) {
> > +        cx->compartment()->setObjectPendingMetadata(obj);
> > +    } else {
> > +        SetNewObjectMetadata(cx, obj);
> > +    }
> 
> Isn't obj here always of class ArrayObject::class_? Seems like the
> shouldDelayMetadataCallback check is not needed.

Good catch.

Comment 26

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 1: Add byte size to the allocation log

Comment 27

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 2: Add a test for byte sizes in the allocations log

Comment 28

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 3: Document the size property in the allocations log

Comment 29

[Shu-yu Guo [:shu]]

Comment on attachment 8623358 [details] [diff] [review]
Part 1: Add byte size to the allocation log

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

Great patch.

::: js/src/jscompartment.cpp
@@ +968,5 @@
> +        return;
> +
> +    if (!cx->isExceptionPending() && cx->compartment()->hasObjectPendingMetadata()) {
> +        JSObject* obj = cx->compartment()->objectMetadataState.as<PendingMetadata>();
> +        cx->compartment()->objectMetadataState = prevState;

Is it important to set cx->compartment()->objectMetadataState before calling SetNewObjectMetadata? If not, could factor out the prevState assignment out of both branches.

::: js/src/jscompartment.h
@@ +128,5 @@
> +// We must ensure that all newly allocated JSObjects get their metadata
> +// set. However, metadata callbacks may require the new object be in a sane
> +// state (eg, have its reserved slots initialized so they can get the
> +// sizeOfExcludingThis of the object). Therefore, for objects of certain
> +// JSClasses (those demarked with JSCLASS_DELAY_METADATA_CALLBACK), it is not

Nit: demarked -> marked

@@ +130,5 @@
> +// state (eg, have its reserved slots initialized so they can get the
> +// sizeOfExcludingThis of the object). Therefore, for objects of certain
> +// JSClasses (those demarked with JSCLASS_DELAY_METADATA_CALLBACK), it is not
> +// safe for the allocation paths to call the object metadata callback
> +// immediately. Instead, the JSClass-specific "factory" function up the stack

Maybe just call them constructors? Not sure -- the factory terminology generally isn't used in SM though.

@@ +174,5 @@
> +//     DelayMetadata -------------------------> PendingMetadata
> +//                         via allocation
> +//
> +// In the presence of internal errors, we do not set the new object's metadata
> +// (if it was even allocated) and reset to the previous state on the stack.

Really great block comment.

@@ +182,5 @@
> +using PendingMetadata = ReadBarrieredObject;
> +
> +using NewObjectMetadataState = mozilla::Variant<ImmediateMetadata,
> +                                                DelayMetadata,
> +                                                PendingMetadata>;

Dang yo this is some nice C++ right here.

@@ +184,5 @@
> +using NewObjectMetadataState = mozilla::Variant<ImmediateMetadata,
> +                                                DelayMetadata,
> +                                                PendingMetadata>;
> +
> +class MOZ_STACK_CLASS AutoSetNewObjectMetadata : private JS::CustomAutoRooter

Dannng private inheritance.

@@ +189,5 @@
> +{
> +    MOZ_DECL_USE_GUARD_OBJECT_NOTIFIER;
> +
> +    JSContext* cx;
> +    NewObjectMetadataState prevState;

Nit: SM has the questionable style of appending _ to private fields.

@@ +196,5 @@
> +    void operator=(const AutoSetNewObjectMetadata& aOther) = delete;
> +
> +  protected:
> +    virtual void trace(JSTracer* trc) override {
> +        if (prevState.is<PendingMetadata>())

Nit: { }

::: js/src/vm/ArrayObject-inl.h
@@ +20,5 @@
>  #include "vm/TypeInference-inl.h"
>  
>  namespace js {
>  
> +using mozilla::Move;

Preferably no using declarations inside headers.

::: js/src/vm/ArrayObject.h
@@ -52,5 @@
> -    createArray(ExclusiveContext* cx,
> -                gc::InitialHeap heap,
> -                HandleShape shape,
> -                HandleObjectGroup group,
> -                HeapSlot* elements);

Was this method dead?

Comment 30

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Shu-yu Guo [:shu] from comment #29)
> Comment on attachment 8623358 [details] [diff] [review]
> Part 1: Add byte size to the allocation log
> 
> Review of attachment 8623358 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Great patch.

Thank you!

> ::: js/src/jscompartment.cpp
> @@ +968,5 @@
> > +        return;
> > +
> > +    if (!cx->isExceptionPending() && cx->compartment()->hasObjectPendingMetadata()) {
> > +        JSObject* obj = cx->compartment()->objectMetadataState.as<PendingMetadata>();
> > +        cx->compartment()->objectMetadataState = prevState;
> 
> Is it important to set cx->compartment()->objectMetadataState before calling
> SetNewObjectMetadata? If not, could factor out the prevState assignment out
> of both branches.

Yes, SetNewObjectMetadata asserts that the state is not PendingMetadata in order to catch this invalid series of events:

* metadata-delayed object A allocated
* normal object B allocated
* metadata callback called for B

The metadata callback for A should always be called before B.

> @@ +130,5 @@
> > +// state (eg, have its reserved slots initialized so they can get the
> > +// sizeOfExcludingThis of the object). Therefore, for objects of certain
> > +// JSClasses (those demarked with JSCLASS_DELAY_METADATA_CALLBACK), it is not
> > +// safe for the allocation paths to call the object metadata callback
> > +// immediately. Instead, the JSClass-specific "factory" function up the stack
> 
> Maybe just call them constructors? Not sure -- the factory terminology
> generally isn't used in SM though.

Ok.

> @@ +174,5 @@
> > +//     DelayMetadata -------------------------> PendingMetadata
> > +//                         via allocation
> > +//
> > +// In the presence of internal errors, we do not set the new object's metadata
> > +// (if it was even allocated) and reset to the previous state on the stack.
> 
> Really great block comment.

Thank you.

> @@ +189,5 @@
> > +{
> > +    MOZ_DECL_USE_GUARD_OBJECT_NOTIFIER;
> > +
> > +    JSContext* cx;
> > +    NewObjectMetadataState prevState;
> 
> Nit: SM has the questionable style of appending _ to private fields.

I thought that was only for when there is a getter for the thing with the same name? Ie private field `Foo foo_` and getter `Foo foo()`.

Is that incorrect?

> ::: js/src/vm/ArrayObject.h
> @@ -52,5 @@
> > -    createArray(ExclusiveContext* cx,
> > -                gc::InitialHeap heap,
> > -                HandleShape shape,
> > -                HandleObjectGroup group,
> > -                HeapSlot* elements);
> 
> Was this method dead?

Yup.

Comment 31

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 1: Add byte size to the allocation log

Comment 32

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 2: Add a test for byte sizes in the allocations log

Comment 33

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 3: Document the size property in the allocations log

Comment 34

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=34736e36f9d9

Comment 35

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Hm, seems there were some CGC failures. Looking into it.

Comment 36

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Can't seem to reproduce those failures. Trying another push, after rebasing on latest m-c: https://treeherder.mozilla.org/#/jobs?repo=try&revision=282722e9ba97

Comment 37

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 4: Fix object-pending-metadata root marking

Comment 38

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #37)
> Created attachment 8631307 [details] [diff] [review]
> Part 4: Fix object-pending-metadata root marking

So the problem was that JSCompartment::markRoots wasn't called for minor GCs because the old set of things it marked didn't need to be marked in a minor GC. That changed with the introduction of the object pending metadata, so this is a small shuffling of marking code that fixes the problem.

This is a pretty small change, but not quite small enough that I felt comfortable carrying over r+. Especially when dealing with marking subtleties.

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

Comment 39

[Shu-yu Guo [:shu]]

Comment on attachment 8631307 [details] [diff] [review]
Part 4: Fix object-pending-metadata root marking

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

This makes an already confusing JSCompartment marking situation slightly more confusing.

It's fine for now since it shouldn't block landing this bug, but please clean it up when possible.

::: js/src/jscompartment.cpp
@@ +526,3 @@
>  
> +    // Globals aren't nursery allocated so there's no need to do this for minor
> +    // GCs.

This comment isn't accurate since we are obviously also marking the jitCompartment below. Could you move this comment to be above the objectMetadataState and say that only the metadata object can be nursery-allocated?

Comment 40

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f8bd1cf9a4fa
https://hg.mozilla.org/integration/mozilla-inbound/rev/49c3c756541b
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd65028f1ae2
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b9abd8b5150

Comment 41

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f8bd1cf9a4fa
https://hg.mozilla.org/mozilla-central/rev/49c3c756541b
https://hg.mozilla.org/mozilla-central/rev/fd65028f1ae2
https://hg.mozilla.org/mozilla-central/rev/1b9abd8b5150