Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(6)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 months, 1 week ago by jbroman
Modified:
4 months, 3 weeks 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
5 months ago (2017-03-18 23:22:43 UTC) #15
haraken
A couple of questions: - In terms of binary size, maybe should we consider generalizing ...
5 months 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 ...
5 months 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 ...
5 months 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 ...
5 months 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 ...
5 months ago (2017-03-22 15:05:26 UTC) #23
jochen (gone - plz use gerrit)
Maybe v8 and Blink can use the same hash for strings, then we can easily ...
5 months 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 ...
5 months ago (2017-03-22 15:21:15 UTC) #25
jochen (gone - plz use gerrit)
The idea of sharing the hash is that if you already have an AtomicString or ...
4 months, 4 weeks 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 ...
4 months, 4 weeks ago (2017-03-23 16:00:47 UTC) #27
jochen (gone - plz use gerrit)
On 2017/03/23 at 16:00:47, jbroman wrote: > On 2017/03/23 at 15:50:37, jochen wrote: > > ...
4 months, 4 weeks 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 ...
4 months, 4 weeks 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 ...
4 months, 4 weeks 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 ...
4 months, 4 weeks 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 ...
4 months, 4 weeks 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 ...
4 months, 3 weeks 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 :) ...
4 months, 3 weeks 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 ...
4 months, 3 weeks 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 ...
4 months, 3 weeks 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 ...
4 months, 3 weeks 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
4 months, 3 weeks ago (2017-03-30 15:48:58 UTC) #41
commit-bot: I haz the power
4 months, 3 weeks 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 b40b6558b