Jon Coppeard (:jonco) (PTO until 14th September) Assignee
	Description • 7 years ago

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).

	Jon Coppeard (:jonco) (PTO until 14th September) Assignee
	Comment 1 • 7 years ago

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.

	Jon Coppeard (:jonco) (PTO until 14th September) Assignee
	Comment 2 • 7 years ago

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.

	André Bargull [:anba]
	Comment 3 • 7 years ago

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(...)?

	André Bargull [:anba]
	Comment 4 • 7 years ago

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 ?

	André Bargull [:anba]
	Comment 5 • 7 years ago

(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. :-)

	Jon Coppeard (:jonco) (PTO until 14th September) Assignee
	Comment 6 • 7 years ago

(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.

	Jon Coppeard (:jonco) (PTO until 14th September) Assignee
	Comment 7 • 7 years ago

(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

	André Bargull [:anba]
	Comment 8 • 7 years ago

(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!

	Pulsebot
	Comment 9 • 7 years ago

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

	Andrei Ciure[:aciure]
	Comment 10 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/ebe3e6a61f67
https://hg.mozilla.org/mozilla-central/rev/6ecf891cc6ce