Closed Bug 1129226 Opened 5 years ago Closed 5 years ago

Refactor ObjectGroup code

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: bhackett1024, Unassigned)

References

Details

Attachments

(1 file)

Attached 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
Blocks: 1130708
You need to log in before you can comment on or make changes to this bug.