Bug 194583 - Cache the results of BytecodeGenerator::getVariablesUnderTDZ
Summary: Cache the results of BytecodeGenerator::getVariablesUnderTDZ
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-02-12 22:10 PST by Saam Barati
Modified: 2019-02-14 16:06 PST (History)
15 users (show)

See Also:


Attachments
patch (16.92 KB, patch)
2019-02-12 23:18 PST, Saam Barati
ysuzuki: review+
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
patch for landing (16.91 KB, patch)
2019-02-14 15:39 PST, Saam Barati
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2019-02-12 22:10:32 PST
To speed up repeated computations
Comment 1 Radar WebKit Bug Importer 2019-02-12 22:12:01 PST
<rdar://problem/48028140>
Comment 2 Saam Barati 2019-02-12 23:18:58 PST
Created attachment 361900 [details]
patch
Comment 3 Saam Barati 2019-02-13 01:38:35 PST
Ugh, our bytecode-cache tests are slightly wrong. It expects that scripts never change -- however, my test is precisely a script that changes on each invocation.
Comment 4 EWS Watchlist 2019-02-13 02:01:01 PST
Comment on attachment 361900 [details]
patch

Attachment 361900 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/11132300

New failing tests:
microbenchmarks/cache-get-variables-under-tdz-in-bytecode-generator.js.bytecode-cache
apiTests
Comment 5 Yusuke Suzuki 2019-02-14 14:23:56 PST
Comment on attachment 361900 [details]
patch

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

r=me with one suggestion.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:1317
> +        uint64_t m_tdzStackEpoch { 1 };
> +        uint64_t m_cachedVariablesUnderTDZEpoch { 0 };
> +        CompactVariableMap::Handle m_cachedVariablesUnderTDZ;

Why not clearing m_cachedVariablesUnderTDZ when changing tdz stack instead of tracking the change with epoch? (Adding `explicit operator bool()` to CompactVariableMap::Handle, and clear the cached handle instead of incrementing epoch).
Comment 6 Saam Barati 2019-02-14 14:27:38 PST
Comment on attachment 361900 [details]
patch

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

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:1317
>> +        CompactVariableMap::Handle m_cachedVariablesUnderTDZ;
> 
> Why not clearing m_cachedVariablesUnderTDZ when changing tdz stack instead of tracking the change with epoch? (Adding `explicit operator bool()` to CompactVariableMap::Handle, and clear the cached handle instead of incrementing epoch).

Sounds good. Not sure why I went with a more complicated implementation. This is much nicer.
Comment 7 Saam Barati 2019-02-14 15:39:28 PST
Created attachment 362073 [details]
patch for landing
Comment 8 WebKit Commit Bot 2019-02-14 16:06:36 PST
Comment on attachment 362073 [details]
patch for landing

Clearing flags on attachment: 362073

Committed r241571: <https://trac.webkit.org/changeset/241571>
Comment 9 WebKit Commit Bot 2019-02-14 16:06:38 PST
All reviewed patches have been landed.  Closing bug.