Bug 194583

Summary: Cache the results of BytecodeGenerator::getVariablesUnderTDZ
Product: WebKit Reporter: Saam Barati <sbarati>
Component: JavaScriptCoreAssignee: Saam Barati <sbarati>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, commit-queue, ews-watchlist, fpizlo, ggaren, gskachkov, guijemont, keith_miller, mark.lam, msaboff, rmorisset, ticaiolima, tzagallo, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
ysuzuki: review+, ews-watchlist: commit-queue-
patch for landing none

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.