Closed Bug 1420412 Opened 3 years ago Closed 3 years ago

ModuleObject has too many reserved slots

Categories

(Core :: JavaScript Engine, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: jonco, Assigned: jonco)

Details

Attachments

(2 files)

Currently ModuleObject has 18 reserved slots, which necessitates an extra allocation (the largest number of slots we can store inline with an object is 16).
Patch to use a single slot for the module's environment object.

According to the spec this isn't created until the module is instantiated, but we create it when we compile the module.  Currently we store it in InitialEnvironmentSlot and copy it to EnvironmentSlot when it is supposed to be created, but we can just store it in the latter slot straight away and check the module's status and return null if it shouldn't exist yet.
Attachment #8931675 - Flags: review?(andrebargull)
Patch to move the namespace exports list and namespace bindings to the namespace object.

This uses the proxy reserved slots to store these things rather than putting them on the module object itself.  This makes more sense anyway.

This takes the slot count down to 15.
Attachment #8931676 - Flags: review?(andrebargull)
Priority: -- → P3
Comment on attachment 8931675 [details] [diff] [review]
bug1420412-env-slot

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

Looks reasonable. It seems like we still need to implement the changes from this PR https://github.com/tc39/ecma262/pull/1006. Is that correct?

::: js/src/builtin/ModuleObject.cpp
@@ +1040,5 @@
>  /* static */ bool
>  ModuleObject::instantiateFunctionDeclarations(JSContext* cx, HandleModuleObject self)
>  {
>  #ifdef DEBUG
> +    MOZ_ASSERT(self->status() == MODULE_STATUS_INSTANTIATING);

This is great because it helps to understand when this function is called. Can we add a similar assertion to ModuleObject::execute(...)?
Attachment #8931675 - Flags: review?(andrebargull) → review+
Comment on attachment 8931676 [details] [diff] [review]
bug1420412-namespace-slots

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

::: js/src/builtin/ModuleObject.cpp
@@ +1096,5 @@
>      MOZ_ASSERT(!self->namespace_());
>      MOZ_ASSERT(exports->is<ArrayObject>());
>  
> +    Zone* zone = cx->zone();
> +    mozilla::UniquePtr<IndirectBindingMap> bindings(zone->new_<IndirectBindingMap>(zone)); // todo

todo?

::: js/src/builtin/SelfHostingDefines.h
@@ +103,5 @@
>  #define MODULE_OBJECT_ENVIRONMENT_SLOT        1
>  #define MODULE_OBJECT_STATUS_SLOT             3
>  #define MODULE_OBJECT_ERROR_SLOT              4
> +#define MODULE_OBJECT_DFS_INDEX_SLOT          13
> +#define MODULE_OBJECT_DFS_ANCESTOR_INDEX_SLOT 14

Maybe we should add static_asserts for these constants similar to https://searchfox.org/mozilla-central/rev/a5d613086ab4d0578510aabe8653e58dc8d7e3e2/js/src/builtin/ModuleObject.cpp#25-29 ?
Attachment #8931676 - Flags: review?(andrebargull) → review+
(In reply to André Bargull [:anba] from comment #3)
> Comment on attachment 8931675 [details] [diff] [review]
> bug1420412-env-slot
> 
> Review of attachment 8931675 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks reasonable. It seems like we still need to implement the changes from
> this PR https://github.com/tc39/ecma262/pull/1006. Is that correct?

NVM, I didn't see you already filed bug 1420420. :-)
(In reply to André Bargull [:anba] from comment #4)
> This is great because it helps to understand when this function is called.
> Can we add a similar assertion to ModuleObject::execute(...)?

Good idea, I'll do that.
(In reply to André Bargull [:anba] from comment #4)
> todo?

Oh thanks, that was to remind me to change this to use 'make_unique' which cuts out some boilerplate.

> Maybe we should add static_asserts for these constants similar to
> https://searchfox.org/mozilla-central/rev/
> a5d613086ab4d0578510aabe8653e58dc8d7e3e2/js/src/builtin/ModuleObject.cpp#25-
> 29 ?

We have static asserts here, if this is what you mean: https://searchfox.org/mozilla-central/source/js/src/builtin/ModuleObject.h#262
(In reply to Jon Coppeard (:jonco) from comment #7)
> > Maybe we should add static_asserts for these constants similar to
> > https://searchfox.org/mozilla-central/rev/
> > a5d613086ab4d0578510aabe8653e58dc8d7e3e2/js/src/builtin/ModuleObject.cpp#25-
> > 29 ?
> 
> We have static asserts here, if this is what you mean:
> https://searchfox.org/mozilla-central/source/js/src/builtin/ModuleObject.
> h#262

Yes, that's what I meant. Thanks!
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ebe3e6a61f67
Use a single slot to store the module environment record r=anba
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ecf891cc6ce
Move namespace related data to the module's namespace object r=anba
https://hg.mozilla.org/mozilla-central/rev/ebe3e6a61f67
https://hg.mozilla.org/mozilla-central/rev/6ecf891cc6ce
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.