1129226 - Refactor ObjectGroup code

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

Attachment: patch

Bug 1125930 renamed TypeObject to ObjectGroup, but much of the functionality of ObjectGroup is still pretty separate from what the rest of jsinfer.* is doing.

The attached patch adds new files vm/ObjectGroup.{h,cpp} which contain ObjectGroup and a new class, ObjectGroupCompartment, to manage the various tables indexing the ObjectGroups.  This class has the tables previously in TypeCompartment (which this patch deletes) and JSCompartment.  This patch also tries to clean up the various methods accessing these tables by centralizing them in ObjectGroup and ObjectGroupCompartment.

Comment 1

[Brian Hackett [Laid off!]]

Oh, and there are some other changes here too to fix build breakage with unified compilation disabled.

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 8558820 [details] [diff] [review]
patch

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

Nice idea.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +7255,5 @@
>                      // interpreter or JIT compiler fetches the template, it should
> +                    // use ObjectGroup::getOrFixupCopyOnWriteObject to make sure the
> +                    // type for the template is accurate. We don't do this here as we
> +                    // want to use ObjectGroup::allocationSiteGroup, which requires a
> +                    // finished script.

s/type/group/ a few times

::: js/src/vm/JSONParser.cpp
@@ -580,5 @@
>  JSObject *
>  JSONParserBase::createFinishedObject(PropertyVector &properties)
>  {
>      /*
>       * Look for an existing cached type and shape for objects with this set of

Nit: cached group

@@ -584,5 @@
>       * Look for an existing cached type and shape for objects with this set of
>       * properties.
>       */
>      {
> -        JSObject *obj = cx->compartment()->types.newTypedObject(cx, properties.begin(),

Hm newTypedObject was confusing.

@@ +610,5 @@
>              return nullptr;
>      }
>  
>      /*
>       * Try to assign a new type to the object with type information for its

new group

::: js/src/vm/UbiNode.cpp
@@ +67,5 @@
>        case JSTRACE_BASE_SHAPE:  construct(static_cast<js::BaseShape *>(ptr));         break;
>        case JSTRACE_JITCODE:     construct(static_cast<js::jit::JitCode *>(ptr));      break;
>        case JSTRACE_LAZY_SCRIPT: construct(static_cast<js::LazyScript *>(ptr));        break;
>        case JSTRACE_SHAPE:       construct(static_cast<js::Shape *>(ptr));             break;
> +      case JSTRACE_OBJECT_GROUP: construct(static_cast<js::ObjectGroup *>(ptr)); break;

Nit: add some whitespace before "break;" so that it lines up

Comment 3

[Luke Wagner [:luke]]

Great patch!  After this patch, it looks like namespace types will have a lot less in it so I was wondering if you've considered removing it and putting the contents into 'js'?  Most of the contents are already prefixed with 'Type' and many .cpps already 'using namespace js::types' so, with unified builds, we have to deal with clashes anyway.

Comment 4

[Brian Hackett [Laid off!]]

(In reply to Luke Wagner [:luke] from comment #3)
> Great patch!  After this patch, it looks like namespace types will have a
> lot less in it so I was wondering if you've considered removing it and
> putting the contents into 'js'?  Most of the contents are already prefixed
> with 'Type' and many .cpps already 'using namespace js::types' so, with
> unified builds, we have to deal with clashes anyway.

Yeah, I would like to remove the js::types namespace.

Comment 5

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6bfcb81d3716

Comment 6

[Andrew McCreight [:mccr8]]

Changed the LSan suppression from TypeCompartment::fixObjectGroup to ObjectGroup::fixPlainObjectGroup with rubberstamp from RyanVM over IRC.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d8b19758967b

Comment 7

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

https://hg.mozilla.org/mozilla-central/rev/6bfcb81d3716
https://hg.mozilla.org/mozilla-central/rev/d8b19758967b