Bug 182717 - [JSC] cache TaggedTemplate arrays by callsite rather than by contents
Summary: [JSC] cache TaggedTemplate arrays by callsite rather than by contents
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Caitlin Potter (:caitp)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-02-12 16:06 PST by Caitlin Potter (:caitp)
Modified: 2018-02-13 10:11 PST (History)
9 users (show)

See Also:


Attachments
[JSC] cache TaggedTemplate arrays by callsite rather than by contents (81.83 KB, patch)
2018-02-12 16:11 PST, Caitlin Potter (:caitp)
no flags Details | Formatted Diff | Diff
Patch (93.50 KB, patch)
2018-02-12 21:39 PST, Caitlin Potter (:caitp)
no flags Details | Formatted Diff | Diff
Patch (102.93 KB, patch)
2018-02-13 09:01 PST, Caitlin Potter (:caitp)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Caitlin Potter (:caitp) 2018-02-12 16:06:27 PST
[JSC] cache TaggedTemplate arrays by callsite rather than by contents
Comment 1 Caitlin Potter (:caitp) 2018-02-12 16:11:48 PST
Created attachment 333641 [details]
[JSC] cache TaggedTemplate arrays by callsite rather than by contents
Comment 2 Caitlin Potter (:caitp) 2018-02-12 16:19:10 PST
This patch is based on the v8 implementation @ https://chromium.googlesource.com/v8/v8/+/d3ca0d005077085a30e27415827325ea2216a735 (so, some class/file renaming is influenced by that)

First patch:

- Change BytecodeGenerator mapping from TemplateRegistryKey -> RegisterID to TemplateRegistryKey -> JSTemplateRegistryKey, to prevent creating more JS wrappers than necessary (this probably doesn't buy anything valuable, so no reason not to get rid of this in a later version) --- A new constant pool index is always created, because each TemplateRegistryKey (TemplateObjectDescriptor) gets its own array (per callsite).

- Disable DirectEvalCodeCache if tagged templates occur in code block. This ensures each call to `eval()` will create own template arrays, per spec.

- Delete TemplateRegistry and TemplateRegistryKeyTable

Tests aren't updated yet, wanting to see what will fail before fixing those up or adding new ones.
Comment 3 EWS Watchlist 2018-02-12 20:15:18 PST
Comment on attachment 333641 [details]
[JSC] cache TaggedTemplate arrays by callsite rather than by contents

Attachment 333641 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/6476510

New failing tests:
stress/tagged-template-registry-key.js.ftl-no-cjit-no-inline-validate
stress/tagged-template-registry-key.js.ftl-no-cjit-no-put-stack-validate
stress/tagged-templates-identity.js.ftl-no-cjit-no-inline-validate
stress/tagged-templates-identity.js.ftl-no-cjit-no-put-stack-validate
stress/tagged-template-registry-key.js.no-ftl
stress/tagged-templates-identity.js.no-ftl
stress/template-string-tags-eval.js.dfg-eager
stress/tagged-template-registry-key.js.dfg-maximal-flush-validate-no-cjit
stress/tagged-template-registry-key.js.no-llint
stress/tagged-templates-identity.js.no-llint
stress/template-string-tags-eval.js.no-cjit-collect-continuously
stress/tagged-template-registry-key.js.no-cjit-collect-continuously
stress/tagged-templates-identity.js.no-cjit-collect-continuously
stress/tagged-templates-identity.js.no-cjit-validate-phases
stress/template-string-tags-eval.js.ftl-eager-no-cjit
stress/tagged-template-registry-key.js.no-cjit-validate-phases
stress/tagged-templates-identity.js.dfg-maximal-flush-validate-no-cjit
stress/template-string-tags-eval.js.no-ftl
stress/tagged-templates-identity.js.ftl-eager
stress/template-string-tags-eval.js.dfg-maximal-flush-validate-no-cjit
stress/tagged-template-registry-key.js.ftl-eager
stress/tagged-templates-identity.js.ftl-eager-no-cjit
stress/tagged-template-registry-key.js.ftl-eager-no-cjit
stress/tagged-template-registry-key.js.ftl-no-cjit-b3o1
stress/tagged-templates-identity.js.ftl-no-cjit-b3o1
stress/template-string-tags-eval.js.ftl-no-cjit-validate-sampling-profiler
stress/template-string-tags-eval.js.dfg-eager-no-cjit-validate
stress/template-string-tags-eval.js.ftl-eager
stress/tagged-templates-identity.js.dfg-eager-no-cjit-validate
stress/tagged-template-registry-key.js.dfg-eager-no-cjit-validate
stress/tagged-templates-identity.js.dfg-eager
stress/tagged-templates-identity.js.ftl-no-cjit-small-pool
stress/tagged-template-registry-key.js.default
stress/tagged-templates-identity.js.default
stress/template-string-tags-eval.js.ftl-no-cjit-no-inline-validate
stress/tagged-template-registry-key.js.ftl-eager-no-cjit-b3o1
stress/tagged-templates-identity.js.ftl-eager-no-cjit-b3o1
stress/tagged-template-registry-key.js.dfg-eager
stress/template-string-tags-eval.js.default
stress/tagged-template-registry-key.js.ftl-no-cjit-small-pool
stress/template-string-tags-eval.js.ftl-eager-no-cjit-b3o1
stress/template-string-tags-eval.js.ftl-no-cjit-no-put-stack-validate
stress/template-string-tags-eval.js.ftl-no-cjit-b3o1
stress/template-string-tags-eval.js.ftl-no-cjit-small-pool
stress/template-string-tags-eval.js.no-llint
stress/tagged-template-registry-key.js.ftl-no-cjit-validate-sampling-profiler
stress/template-string-tags-eval.js.no-cjit-validate-phases
stress/tagged-templates-identity.js.ftl-no-cjit-validate-sampling-profiler
Comment 4 Caitlin Potter (:caitp) 2018-02-12 21:39:01 PST
Created attachment 333671 [details]
Patch
Comment 5 Yusuke Suzuki 2018-02-13 01:38:03 PST
Comment on attachment 333671 [details]
Patch

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

r=me with test262.yaml update. Since test262 includes tests for this, we should update test262.yaml.

> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp:75
> +    , m_hasTaggedTemplates(false)

Drop this since it is not used.

> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h:457
> +    unsigned m_hasTaggedTemplates : 1;

Ditto.
Comment 6 Caitlin Potter (:caitp) 2018-02-13 09:01:32 PST
Created attachment 333693 [details]
Patch

Remove unneeded member from UnlinkedCodeBlock and update test262.yaml
Comment 7 Yusuke Suzuki 2018-02-13 09:02:51 PST
Comment on attachment 333693 [details]
Patch

r=me
Comment 8 WebKit Commit Bot 2018-02-13 10:10:35 PST
Comment on attachment 333693 [details]
Patch

Clearing flags on attachment: 333693

Committed r228422: <https://trac.webkit.org/changeset/228422>
Comment 9 WebKit Commit Bot 2018-02-13 10:10:37 PST
All reviewed patches have been landed.  Closing bug.
Comment 10 Radar WebKit Bug Importer 2018-02-13 10:11:21 PST
<rdar://problem/37501758>