Bug 148896 – [ES6] Cache the resolution result in JSModuleRecord

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Yusuke Suzuki]

When resolveExport is succeeded, we should cache the result in JSModuleRecord since it is frequently called.

[Yusuke Suzuki]

Created attachment 261026 [details]
Patch

[Yusuke Suzuki]

Created attachment 261027 [details]
Patch

[Yusuke Suzuki]

Created attachment 261029 [details]
Patch

debug build fix

[Yusuke Suzuki]

Created attachment 261033 [details]
Patch

[Yusuke Suzuki]

OK, the patch is ready.

[Saam Barati]

Comment on attachment 261033 [details]
Patch

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

r=me with comments.

> Source/JavaScriptCore/runtime/JSModuleRecord.cpp:362
> +    {

I think official style is to have this on the line above. (Even if the style checker complains, unless we've recently changed the style guide on this).

> Source/JavaScriptCore/runtime/JSModuleRecord.h:168
> +    Optional<Resolution> tryGetCachedResolsion(UniquedStringImpl* exportName);

tryGetCachedResolsion => tryGetCachedResolution

[Yusuke Suzuki]

Created attachment 261050 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 261050 [details]
Patch

I'll add the test that leverages namespace object.

[Yusuke Suzuki]

Created attachment 261076 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 261033 [details]
Patch

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

>> Source/JavaScriptCore/runtime/JSModuleRecord.cpp:362
>> +    {
> 
> I think official style is to have this on the line above. (Even if the style checker complains, unless we've recently changed the style guide on this).

Thanks. I'll fix this.

>> Source/JavaScriptCore/runtime/JSModuleRecord.h:168
>> +    Optional<Resolution> tryGetCachedResolsion(UniquedStringImpl* exportName);
> 
> tryGetCachedResolsion => tryGetCachedResolution

Oops. Fixing...

[Yusuke Suzuki]

Created attachment 261080 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/runtime/JSModuleRecord.cpp:599:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 39 files


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

[Yusuke Suzuki]

OK, patch is ready.

[Yusuke Suzuki]

Created attachment 261081 [details]
Patch

[Yusuke Suzuki]

Oops, one thing is fixed. Now, the patch is ready.

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/runtime/JSModuleRecord.cpp:599:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 39 files


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

[Saam Barati]

Comment on attachment 261081 [details]
Patch

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

r=me

> Source/JavaScriptCore/runtime/JSModuleRecord.cpp:300
> +    // Technically, the all JSModuleRecord have the Map<ExportName, Resolution> for caching.

"the all JSModuleRecord" => "all the JSModuleRecords"

> Source/JavaScriptCore/runtime/JSModuleRecord.cpp:305
> +    //  - *cacheable* means that we can cache the gain result because the result is the same to the result
> +    //    that will be produced when starting from this node. This means that the following 2 is the same.

I would write this as:
*cacheable* means that traversing to this node from a path will produce the same results as starting from this node.

> Source/JavaScriptCore/runtime/JSModuleRecord.cpp:307
> +    //      1. The resolving route from this node when tracing the nodes from the other starting node.
> +    //      2. The resolving route from this node when starting from this node.

Not needed given the above sentence.

> Source/JavaScriptCore/runtime/JSModuleRecord.cpp:309
> +    //    Here, we define the resovling route. We represent [?] as the module that has the searched local binding.

I think you mean [?] as something that is resolved with a local binding, right? Not just searched.

> Source/JavaScriptCore/runtime/JSModuleRecord.cpp:325
> +    //  - Cacheability depends on the resolving route from this node. Not depend on the node.
> +    //    The node is sometimes cacheable. And sometimes non-cacheable.
> +    //    The resolving route depends on the starting node.

I think this can be written as:
"The cache ability of a node depends on the resolving route from this node."

> Source/JavaScriptCore/runtime/JSModuleRecord.cpp:329
> +    // By the definition of the cacheable, the starting point is always cacheable.

not needed.

> Source/JavaScriptCore/runtime/JSModuleRecord.cpp:331
> +    //  => The starting point is always cacheable.

not needed.

> Source/JavaScriptCore/runtime/JSModuleRecord.cpp:341
> +    //  => The module that has the searched local resolution is always cacheable.

I would write this as:
"A module that has resolved a local binding is always cacheable."

> Source/JavaScriptCore/runtime/JSModuleRecord.cpp:343
> +    // 3. Indirect exported bindings are cacheable if there is only non-star links.

"is only non-star" => "are no star"

> Source/JavaScriptCore/runtime/JSModuleRecord.cpp:346
> +    //  If there are only non-star links, it means that there is *no branch* in the module dependency graph.
> +    //  This *no branch* feature makes the all module cachable.

I would explain what you mean by "branch" here.
I.e, if we traverse one star link (even if we successfully resolve that start link),
we must still traverse all other star links. I would also explain we don't run into
this when resolving a local/indirect link. When resolving a local/indirect link,
we won't traverse any star links.

> Source/JavaScriptCore/runtime/JSModuleRecord.cpp:375
> +    //  should already finish the resolution when reaching this *once-visited* node.

"should already finish the resolution when reaching this *once-visited* node" => "should have already finished the resolution before reaching this *once-visited* node"

> Source/JavaScriptCore/runtime/JSModuleRecord.cpp:427
> +    //  The star link makes the node conditional.
> +    //
> +    //       (A) *--> [B]
> +    //            |    ^
> +    //            |    |
> +    //            +-> (C) *-> (D)
> +    //                 ^
> +    //                 |
> +    //                (E)
> +    //                 ^
> +    //                 |
> +    //                 @
> +    //
> +    //  When starting from @,
> +    //
> +    //    1. (E) will follow the link to (C).
> +    //    2. Looking (C), it has star link, but it also has the indirect link. So once we follow the link to [B].
> +    //    3. [B] is resolved. And it is cacheable by 1.
> +    //    4. (C) is resolved. And since we don't follow any star link during this, (C) is cacheable.
> +    //    5. (E) is cacheable by definition.
> +    //
> +    //  But, in the following,
> +    //
> +    //  @ -> (A) *--> [B]
> +    //            |    ^
> +    //            |    |
> +    //            +-> (C) *-> (D)
> +    //                 ^
> +    //                 |
> +    //                (E)
> +    //
> +    //    1. (A) will follow the star links.
> +    //    2. [B] is resolved & cacheable by definition.
> +    //    3. In (C), we attempt to follow the link to [B], but we cannot since [B] is already *once-visited*.
> +    //    4. So, we fall back to the star link to (D).
> +    //    5. (D) is not resolved. So (C) is not resolved.
> +    //
> +    //  In the previous one, (C) is cacheable. But in the above route, (C) becomes uncacheable.
> +    //  So in the second trial, we should not retrieve the result from (C) when visiting (C).
> +    //  Here, we can observe the one thing. In some situation, we should not retrieve the cached result
> +    //  from the node.

I think we don't need these examples. The below example explains this more simply.

> Source/JavaScriptCore/runtime/JSModuleRecord.cpp:468
> +    //  resolution. Because if they have the resolution, we should already finish the tracing.

"Because if they have the resolution, we should already finish the tracing." => "Because if they had a local/indirect resolution, we should have already finished the tracing".

> Source/JavaScriptCore/runtime/JSModuleRecord.cpp:472
> +    //  of the link is always not-found since (A) does not have any location resolution.

"location" => "local"

> Source/JavaScriptCore/runtime/JSModuleRecord.cpp:479
> +    // 5. Once we see the star links, even if it does not used yet, we should disable caching.

"Once we see the star links, even if it does not used yet, we should disable caching." => "Once we see start links, even if we have not yet traversed that star link path, we should disable caching".

> Source/JavaScriptCore/runtime/JSModuleRecord.cpp:481
> +    //  And there is another edge case.

"And there is another edge case." => "Here is the reason why:"

> Source/JavaScriptCore/runtime/JSModuleRecord.cpp:517
> +    //  To avoid this, once we see the star links, even if it does not used yet, we should disable caching.

Not needed.

> Source/JavaScriptCore/runtime/JSModuleRecord.cpp:519
> +    //  => Once we see the star links, even if it does not used yet, we should disable caching.

Not needed.

> Source/JavaScriptCore/runtime/JSModuleRecord.cpp:521
> +    // To summarize the observation.

"observation" => "observations"

> Source/JavaScriptCore/runtime/JSModuleRecord.cpp:616
> +            //  => Once we see the star links, even if it does not used yet, we should disable caching.

rename to my above suggestion (or something similar).

[Yusuke Suzuki]

Comment on attachment 261081 [details]
Patch

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

Thanks! I've reflected your comments :)

>> Source/JavaScriptCore/runtime/JSModuleRecord.cpp:300
>> +    // Technically, the all JSModuleRecord have the Map<ExportName, Resolution> for caching.
> 
> "the all JSModuleRecord" => "all the JSModuleRecords"

Fixed.

>> Source/JavaScriptCore/runtime/JSModuleRecord.cpp:305
>> +    //    that will be produced when starting from this node. This means that the following 2 is the same.
> 
> I would write this as:
> *cacheable* means that traversing to this node from a path will produce the same results as starting from this node.

Fixed.

>> Source/JavaScriptCore/runtime/JSModuleRecord.cpp:307
>> +    //      2. The resolving route from this node when starting from this node.
> 
> Not needed given the above sentence.

Dropped.

>> Source/JavaScriptCore/runtime/JSModuleRecord.cpp:309
>> +    //    Here, we define the resovling route. We represent [?] as the module that has the searched local binding.
> 
> I think you mean [?] as something that is resolved with a local binding, right? Not just searched.

[?] means the module that has a local binding for the current query. I've dropped the word "searched".

>> Source/JavaScriptCore/runtime/JSModuleRecord.cpp:329
>> +    // By the definition of the cacheable, the starting point is always cacheable.
> 
> not needed.

Dropped.

>> Source/JavaScriptCore/runtime/JSModuleRecord.cpp:331
>> +    //  => The starting point is always cacheable.
> 
> not needed.

Dropped.

>> Source/JavaScriptCore/runtime/JSModuleRecord.cpp:341
>> +    //  => The module that has the searched local resolution is always cacheable.
> 
> I would write this as:
> "A module that has resolved a local binding is always cacheable."

Fixed.

>> Source/JavaScriptCore/runtime/JSModuleRecord.cpp:343
>> +    // 3. Indirect exported bindings are cacheable if there is only non-star links.
> 
> "is only non-star" => "are no star"

Fixed.

>> Source/JavaScriptCore/runtime/JSModuleRecord.cpp:346
>> +    //  This *no branch* feature makes the all module cachable.
> 
> I would explain what you mean by "branch" here.
> I.e, if we traverse one star link (even if we successfully resolve that start link),
> we must still traverse all other star links. I would also explain we don't run into
> this when resolving a local/indirect link. When resolving a local/indirect link,
> we won't traverse any star links.

Thanks. I've inserted your explanation. And I've inserted the following paragraph.

And since the module can hold only one local/indirect link for the specific export name (if there
are multiple local/indirect links that has the same export name, it should be syntax error in the
parsing phase.), there is no multiple outgoing links from a module.

>> Source/JavaScriptCore/runtime/JSModuleRecord.cpp:375
>> +    //  should already finish the resolution when reaching this *once-visited* node.
> 
> "should already finish the resolution when reaching this *once-visited* node" => "should have already finished the resolution before reaching this *once-visited* node"

Fixed.

>> Source/JavaScriptCore/runtime/JSModuleRecord.cpp:427
>> +    //  from the node.
> 
> I think we don't need these examples. The below example explains this more simply.

Dropped. Thanks.

>> Source/JavaScriptCore/runtime/JSModuleRecord.cpp:468
>> +    //  resolution. Because if they have the resolution, we should already finish the tracing.
> 
> "Because if they have the resolution, we should already finish the tracing." => "Because if they had a local/indirect resolution, we should have already finished the tracing".

Fixed.

>> Source/JavaScriptCore/runtime/JSModuleRecord.cpp:472
>> +    //  of the link is always not-found since (A) does not have any location resolution.
> 
> "location" => "local"

Fixed.

>> Source/JavaScriptCore/runtime/JSModuleRecord.cpp:479
>> +    // 5. Once we see the star links, even if it does not used yet, we should disable caching.
> 
> "Once we see the star links, even if it does not used yet, we should disable caching." => "Once we see start links, even if we have not yet traversed that star link path, we should disable caching".

Fixed. And use it at the title.

>> Source/JavaScriptCore/runtime/JSModuleRecord.cpp:481
>> +    //  And there is another edge case.
> 
> "And there is another edge case." => "Here is the reason why:"

Fixed.

>> Source/JavaScriptCore/runtime/JSModuleRecord.cpp:517
>> +    //  To avoid this, once we see the star links, even if it does not used yet, we should disable caching.
> 
> Not needed.

Dropped.

>> Source/JavaScriptCore/runtime/JSModuleRecord.cpp:519
>> +    //  => Once we see the star links, even if it does not used yet, we should disable caching.
> 
> Not needed.

Dropped.

>> Source/JavaScriptCore/runtime/JSModuleRecord.cpp:521
>> +    // To summarize the observation.
> 
> "observation" => "observations"

Fixed.

>> Source/JavaScriptCore/runtime/JSModuleRecord.cpp:616
>> +            //  => Once we see the star links, even if it does not used yet, we should disable caching.
> 
> rename to my above suggestion (or something similar).

Fixed :D

[Yusuke Suzuki]

Committed r189747: <http://trac.webkit.org/changeset/189747>