Bug 148896 - [ES6] Cache the resolution result in JSModuleRecord
Summary: [ES6] Cache the resolution result in JSModuleRecord
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords:
Depends on:
Blocks: 147340
  Show dependency treegraph
 
Reported: 2015-09-05 01:48 PDT by Yusuke Suzuki
Modified: 2015-09-14 14:15 PDT (History)
3 users (show)

See Also:


Attachments
Patch (9.57 KB, patch)
2015-09-11 16:31 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (9.57 KB, patch)
2015-09-11 16:34 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (9.58 KB, patch)
2015-09-11 16:40 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (14.52 KB, patch)
2015-09-11 16:59 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (34.18 KB, patch)
2015-09-12 00:55 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (45.07 KB, patch)
2015-09-12 22:39 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (43.82 KB, patch)
2015-09-13 02:13 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (43.82 KB, patch)
2015-09-13 02:31 PDT, Yusuke Suzuki
sbarati: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2015-09-05 01:48:20 PDT
When resolveExport is succeeded, we should cache the result in JSModuleRecord since it is frequently called.
Comment 1 Yusuke Suzuki 2015-09-11 16:31:43 PDT
Created attachment 261026 [details]
Patch
Comment 2 Yusuke Suzuki 2015-09-11 16:34:54 PDT
Created attachment 261027 [details]
Patch
Comment 3 Yusuke Suzuki 2015-09-11 16:40:32 PDT
Created attachment 261029 [details]
Patch

debug build fix
Comment 4 Yusuke Suzuki 2015-09-11 16:59:58 PDT
Created attachment 261033 [details]
Patch
Comment 5 Yusuke Suzuki 2015-09-11 17:00:18 PDT
OK, the patch is ready.
Comment 6 Saam Barati 2015-09-11 17:11:06 PDT
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
Comment 7 Yusuke Suzuki 2015-09-12 00:55:45 PDT
Created attachment 261050 [details]
Patch
Comment 8 Yusuke Suzuki 2015-09-12 00:56:56 PDT
Comment on attachment 261050 [details]
Patch

I'll add the test that leverages namespace object.
Comment 9 Yusuke Suzuki 2015-09-12 22:39:19 PDT
Created attachment 261076 [details]
Patch
Comment 10 Yusuke Suzuki 2015-09-13 02:10:52 PDT
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...
Comment 11 Yusuke Suzuki 2015-09-13 02:13:01 PDT
Created attachment 261080 [details]
Patch
Comment 12 WebKit Commit Bot 2015-09-13 02:15:57 PDT
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.
Comment 13 Yusuke Suzuki 2015-09-13 02:20:13 PDT
OK, patch is ready.
Comment 14 Yusuke Suzuki 2015-09-13 02:31:59 PDT
Created attachment 261081 [details]
Patch
Comment 15 Yusuke Suzuki 2015-09-13 02:32:51 PDT
Oops, one thing is fixed. Now, the patch is ready.
Comment 16 WebKit Commit Bot 2015-09-13 02:35:35 PDT
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.
Comment 17 Saam Barati 2015-09-14 13:21:58 PDT
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).
Comment 18 Yusuke Suzuki 2015-09-14 14:08:34 PDT
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
Comment 19 Yusuke Suzuki 2015-09-14 14:15:31 PDT
Committed r189747: <http://trac.webkit.org/changeset/189747>