Refactor ObjectGroup code

RESOLVED FIXED in Firefox 38

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bhackett, Unassigned)

Tracking

Trunk
mozilla38
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox38 fixed)

Details

Attachments

(1 attachment)

Posted patch patchSplinter Review
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.
Attachment #8558820 - Flags: review?(jdemooij)
Oh, and there are some other changes here too to fix build breakage with unified compilation disabled.
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
Attachment #8558820 - Flags: review?(jdemooij) → review+
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.
(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.
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
https://hg.mozilla.org/mozilla-central/rev/6bfcb81d3716
https://hg.mozilla.org/mozilla-central/rev/d8b19758967b
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Blocks: 1130708
You need to log in before you can comment on or make changes to this bug.