Chromium Code Reviews
Help | Chromium Project | Sign in
(2)

Issue 2751263002: [Bindings] Cache handles for dictionary keys on V8PerIsolateData. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 month, 2 weeks ago by jbroman
Modified:
1 month ago
CC:
blink-reviews, blink-reviews-bindings_chromium.org, chromium-reviews, esprehn, Toon Verwaest
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

[Bindings] Cache handles for dictionary keys on V8PerIsolateData. This reduces APK size by about 20 KiB (because the work to make the dictionary keys exists in one place, out-of-line) and saves work looking up keys in the internalized string table. On the DOMMatrix multiply benchmark, this reduces the overhead compared to the polyfill from 2.4x to 1.6x (on my workstation). BUG=685748 Review-Url: https://codereview.chromium.org/2751263002 Cr-Commit-Position: refs/heads/master@{#460824} Committed: https://chromium.googlesource.com/chromium/src/+/53d467c4d4be2da1882d9c17a4384906a7f6d7e2

Patch Set 1 #

Patch Set 2 : win: give each eternal keys helper a unique name #

Patch Set 3 : Merge branch 'master' into dict-eternal-keys #

Patch Set 4 : const #

Patch Set 5 : findOrCreateEternalNameCache #

Unified diffs Side-by-side diffs Delta from patch set Stats (+231 lines, -90 lines) Patch
M third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.h View 1 2 3 4 2 chunks +12 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.cpp View 1 2 3 4 2 chunks +23 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/bindings/templates/dictionary_v8.cpp.tmpl View 1 2 3 4 4 chunks +20 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/bindings/tests/results/core/V8TestDictionary.cpp View 1 2 3 4 78 chunks +125 lines, -76 lines 0 comments Download
M third_party/WebKit/Source/bindings/tests/results/core/V8TestDictionaryDerived.cpp View 1 2 3 4 10 chunks +23 lines, -8 lines 0 comments Download
M third_party/WebKit/Source/bindings/tests/results/core/V8TestInterfaceEventInit.cpp View 1 2 3 4 4 chunks +14 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/bindings/tests/results/core/V8TestPermissiveDictionary.cpp View 1 2 3 4 4 chunks +14 lines, -2 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 44 (22 generated)
jbroman
1 month, 1 week ago (2017-03-18 23:22:43 UTC) #15
haraken
A couple of questions: - In terms of binary size, maybe should we consider generalizing ...
1 month, 1 week ago (2017-03-19 06:09:55 UTC) #18
jbroman
[cc verwaest, who also suggested caching the internalized strings on the bug] On 2017/03/19 at ...
1 month, 1 week ago (2017-03-20 17:45:08 UTC) #19
haraken
On 2017/03/20 17:45:08, jbroman wrote: > [cc verwaest, who also suggested caching the internalized strings ...
1 month, 1 week ago (2017-03-21 02:51:13 UTC) #20
jbroman
On 2017/03/21 at 02:51:13, haraken wrote: > On 2017/03/20 17:45:08, jbroman wrote: > > [cc ...
1 month, 1 week ago (2017-03-22 14:53:21 UTC) #21
haraken
+jochen The question is if we should cache typical V8 strings in the binding side ...
1 month, 1 week ago (2017-03-22 15:05:26 UTC) #23
jochen (slow until May 2)
Maybe v8 and Blink can use the same hash for strings, then we can easily ...
1 month, 1 week ago (2017-03-22 15:09:08 UTC) #24
jbroman
On 2017/03/22 at 15:09:08, jochen wrote: > Maybe v8 and Blink can use the same ...
1 month, 1 week ago (2017-03-22 15:21:15 UTC) #25
jochen (slow until May 2)
The idea of sharing the hash is that if you already have an AtomicString or ...
1 month, 1 week ago (2017-03-23 15:50:37 UTC) #26
jbroman
On 2017/03/23 at 15:50:37, jochen wrote: > The idea of sharing the hash is that ...
1 month, 1 week ago (2017-03-23 16:00:47 UTC) #27
jochen (slow until May 2)
On 2017/03/23 at 16:00:47, jbroman wrote: > On 2017/03/23 at 15:50:37, jochen wrote: > > ...
1 month ago (2017-03-24 09:36:13 UTC) #28
haraken
On 2017/03/24 09:36:13, jochen wrote: > On 2017/03/23 at 16:00:47, jbroman wrote: > > On ...
1 month ago (2017-03-24 12:03:46 UTC) #29
jbroman
On 2017/03/24 at 12:03:46, haraken wrote: > On 2017/03/24 09:36:13, jochen wrote: > > On ...
1 month ago (2017-03-24 13:48:34 UTC) #30
haraken
On 2017/03/24 13:48:34, jbroman wrote: > On 2017/03/24 at 12:03:46, haraken wrote: > > On ...
1 month ago (2017-03-24 16:48:31 UTC) #31
jbroman
On 2017/03/24 at 16:48:31, haraken wrote: > On 2017/03/24 13:48:34, jbroman wrote: > > On ...
1 month ago (2017-03-24 18:01:44 UTC) #32
haraken
Thanks for the detailed explanation. I'm convinced :) The final question: This CL is creating ...
1 month ago (2017-03-27 05:13:25 UTC) #33
jbroman
On 2017/03/27 at 05:13:25, haraken wrote: > Thanks for the detailed explanation. I'm convinced :) ...
1 month ago (2017-03-29 19:23:01 UTC) #34
Toon Verwaest
The approach seems good to me. Note that the idea isn't to just cache the ...
1 month ago (2017-03-30 08:46:32 UTC) #36
haraken
On 2017/03/29 19:23:01, jbroman wrote: > On 2017/03/27 at 05:13:25, haraken wrote: > > Thanks ...
1 month ago (2017-03-30 14:38:40 UTC) #37
jbroman
On 2017/03/30 at 14:38:40, haraken wrote: > On 2017/03/29 19:23:01, jbroman wrote: > > On ...
1 month ago (2017-03-30 15:47:19 UTC) #38
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2751263002/80001
1 month ago (2017-03-30 15:48:58 UTC) #41
commit-bot: I haz the power
1 month ago (2017-03-30 18:12:45 UTC) #44
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/53d467c4d4be2da1882d9c17a438...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cc6ac46