Bug 160590 – JSModuleNamespace object should have IC

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yusuke Suzuki]

Once the module is introduced, we can consider that namespace object is used so frequently.

import * as ns from './A.js'

ns.xxx
ns.yyy
ns.zzz


They should have IC. At that time, the Get IC is necessary. But Put IC is not necessary since

ns.xxx = value;

does not have any effect.

[Saam Barati]

*** Bug 166458 has been marked as a duplicate of this bug. ***

[Yusuke Suzuki]

Created attachment 302051 [details]
Patch

[Yusuke Suzuki]

*** Bug 165725 has been marked as a duplicate of this bug. ***

[WebKit Commit Bot]

Attachment 302051 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:48:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:49:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:50:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:51:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:53:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:54:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 6 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 302052 [details]
Patch

[WebKit Commit Bot]

Attachment 302052 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:48:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:49:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:50:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:51:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:53:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:54:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 6 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 302053 [details]
Patch

[WebKit Commit Bot]

Attachment 302053 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:48:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:49:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:50:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:51:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:53:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:54:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 6 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 302106 [details]
Patch

[Saam Barati]

Comment on attachment 302106 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=302106&action=review

r=me with comments

> Source/JavaScriptCore/ChangeLog:12
> +            When constructing the module namespace object, we already resolves all the resolutions.

"resolve all the resolutions" should be changed to maybe "resolve all the imports"?

> Source/JavaScriptCore/bytecode/GetByIdStatus.cpp:47
> +GetByIdStatus::GetByIdStatus(State state, const AccessCase& accessCase)

This feels weird to assert the only use is a Module namespace. Why not make it a static function that has a name indicating its a module namespace access?

> Source/JavaScriptCore/bytecode/GetByIdStatus.cpp:214
> +            case AccessCase::ModuleNamespaceLoad:
> +                return GetByIdStatus(ModuleNamespace, access);

This is guaranteed to only ever have a single entry in the list? Or this just an optimization to see if this is the only kind of case?

> Source/JavaScriptCore/bytecode/ModuleNamespaceAccessCase.cpp:83
> +#if USE(JSVALUE64)
> +    state.failAndIgnore.append(
> +        jit.branchTest64(CCallHelpers::Zero, valueRegs.payloadGPR()));
> +#else
> +    state.failAndIgnore.append(
> +        jit.branch32(CCallHelpers::Equal, valueRegs.tagGPR(), CCallHelpers::TrustedImm32(JSValue::EmptyValueTag)));
> +#endif

You should use AssemblyHelpers::branchIfEmpty here.

> Source/JavaScriptCore/runtime/JSModuleNamespaceObject.cpp:79
> +    {
> +        unsigned i = 0;
> +        for (const auto& pair : resolutions)
> +            moduleRecordAt(i++).set(vm, this, pair.second.moduleRecord);
> +        m_moduleRecord.set(vm, this, moduleRecord);
> +    }
> +
> +    {
> +        unsigned moduleRecordOffset = 0;
> +        m_names.reserveCapacity(resolutions.size());
> +        for (const auto& pair : resolutions) {
> +            m_names.append(pair.first);
> +            m_exports.add(pair.first.impl(), ExportEntry {
> +                pair.second.localName,
> +                moduleRecordOffset
> +            });
> +            ++moduleRecordOffset;
> +        }
> +    }

Why two loops?

> Source/JavaScriptCore/runtime/JSModuleNamespaceObject.cpp:136
>      slot.disableCaching();

Why not remove this? This seems to barely work now just because the ordering of how we do checks inside Repatch.

> JSTests/ChangeLog:1
> [phone]  Yusuke Suzuki  <[e-mail]>

an you also add tests for:
- transitive exports
- make sure IC works as the binding changes, so perhaps export some function that changes the binding
- Tests for various forms of get_by_id ICs that include more than just a module namespace AccessCase. It'd be good to know what complex get by id status does for these. At a minimum, something like: `let v = (c ?  namespace : otherThing).foo)`.

[Yusuke Suzuki]

Comment on attachment 302106 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=302106&action=review

Thanks!

>> Source/JavaScriptCore/ChangeLog:12
>> +            When constructing the module namespace object, we already resolves all the resolutions.
> 
> "resolve all the resolutions" should be changed to maybe "resolve all the imports"?

It should be "resolve all the exports". Fixed.

>> Source/JavaScriptCore/bytecode/GetByIdStatus.cpp:47
>> +GetByIdStatus::GetByIdStatus(State state, const AccessCase& accessCase)
> 
> This feels weird to assert the only use is a Module namespace. Why not make it a static function that has a name indicating its a module namespace access?

I planned to extend this to accept various kind of accessCase. But sounds fine. I'll create the static function that creates GetByIdStatus for module namespace load.

>> Source/JavaScriptCore/bytecode/GetByIdStatus.cpp:214
>> +                return GetByIdStatus(ModuleNamespace, access);
> 
> This is guaranteed to only ever have a single entry in the list? Or this just an optimization to see if this is the only kind of case?

No, we may encounter multiple access cases. But when we see multiple ModuleNamespace / ModuleNamespace + other types of access cases, this should be TakesSlowPath (and IC should handle it in DFG/FTL). Here, we would like to return special TakesSlowPath (ModuleNamespace) iff accessCases have only one case and it is module namespace load.

>> Source/JavaScriptCore/bytecode/ModuleNamespaceAccessCase.cpp:83
>> +#endif
> 
> You should use AssemblyHelpers::branchIfEmpty here.

Nice! I'll apply the same changes to DFG / JIT.

>> Source/JavaScriptCore/runtime/JSModuleNamespaceObject.cpp:79
>> +    }
> 
> Why two loops?

Oops. This is derived from the previous version of this patch. Merging these loops.

>> Source/JavaScriptCore/runtime/JSModuleNamespaceObject.cpp:136
>>      slot.disableCaching();
> 
> Why not remove this? This seems to barely work now just because the ordering of how we do checks inside Repatch.

Yeah, we can drop this.

>> JSTests/ChangeLog:1
>> [phone]  Yusuke Suzuki  <[e-mail]>
> 
> an you also add tests for:
> - transitive exports
> - make sure IC works as the binding changes, so perhaps export some function that changes the binding
> - Tests for various forms of get_by_id ICs that include more than just a module namespace AccessCase. It'd be good to know what complex get by id status does for these. At a minimum, something like: `let v = (c ?  namespace : otherThing).foo)`.

Sounds fine.

[Yusuke Suzuki]

Comment on attachment 302106 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=302106&action=review

>>> Source/JavaScriptCore/bytecode/GetByIdStatus.cpp:47
>>> +GetByIdStatus::GetByIdStatus(State state, const AccessCase& accessCase)
>> 
>> This feels weird to assert the only use is a Module namespace. Why not make it a static function that has a name indicating its a module namespace access?
> 
> I planned to extend this to accept various kind of accessCase. But sounds fine. I'll create the static function that creates GetByIdStatus for module namespace load.

Or, just taking const ModuleNamespaceAccessCase& is fine. Fixed.

[Saam Barati]

One more suggestion for a test:
Test for the result being the empty value. (I'm not even sure if it will be possible to make such a test that causes us to cache).

[Yusuke Suzuki]

(In reply to comment #13)
> One more suggestion for a test:
> Test for the result being the empty value. (I'm not even sure if it will be
> possible to make such a test that causes us to cache).

Currently, we do not create the cache for such an empty binding.
And once the binding is filled, we can never see the case that this binding becomes empty again.

[Yusuke Suzuki]

Committed r212712: <http://trac.webkit.org/changeset/212712>

[Ryan Haddad]

(In reply to comment #15)
> Committed r212712: <http://trac.webkit.org/changeset/212712>

It looks like this change broke the CLoop build:

https://build.webkit.org/builders/Apple%20Sierra%20LLINT%20CLoop%20%28BuildAndTest%29/builds/1637

[Ryan Haddad]

Reverted r212712 for reason:

This change broke the CLoop build.

Committed r212717: <http://trac.webkit.org/changeset/212717>

[Yusuke Suzuki]

Committed r212818: <http://trac.webkit.org/changeset/212818>