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

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

Created:
1 year, 1 month ago by jbroman
Modified:
1 year 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 #

Messages

Total messages: 44 (22 generated)
jbroman
1 year, 1 month 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 year, 1 month 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 year, 1 month 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 year, 1 month 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 year, 1 month 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 year, 1 month 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 ...
1 year, 1 month 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 year, 1 month 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 ...
1 year, 1 month 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 year, 1 month 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: > > ...
1 year, 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 year, 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 year, 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 year 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 year 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 year 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 year 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 year 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 year 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 year 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 year ago (2017-03-30 15:48:58 UTC) #41
commit-bot: I haz the power
1 year 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...

Powered by Google App Engine
This is Rietveld 408576698