[Bindings] Cache handles for dictionary keys on V8PerIsolateData.

[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

[jbroman, 2017-03-16 00:02:37]

The CQ bit was checked by jbroman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-16 00:03:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-16 00:20:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-16 00:20:26]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[jbroman, 2017-03-16 00:34:02]

The CQ bit was checked by jbroman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-16 00:34:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-16 02:54:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-16 02:54:03]

Dry run: This issue passed the CQ dry run.

[jbroman, 2017-03-18 20:58:13]

The CQ bit was checked by jbroman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-18 20:58:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jbroman, 2017-03-18 21:27:49]

Description was changed from

==========
[Bindings] Cache handles for dictionary keys on V8PerIsolateData.

BUG=685748
==========

to

==========
[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
==========

[jbroman, 2017-03-18 21:27:50]

jbroman@chromium.org changed reviewers:
+ haraken@chromium.org

[jbroman, 2017-03-18 21:45:02]

The CQ bit was checked by jbroman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-18 21:45:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-19 00:13:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-19 00:13:16]

Dry run: This issue passed the CQ dry run.

[haraken, 2017-03-19 06:09:55]

A couple of questions:

- In terms of binary size, maybe should we consider generalizing this
optimization to other V8 strings created from const char*s?

- In terms of performance, the core problem is that v8AtomicString is slow.
Instead of creating a "string cache" on the binding layer, would it be possible
to try to make v8AtomicString faster?

[jbroman, 2017-03-20 17:45:08]

[cc verwaest, who also suggested caching the internalized strings on the bug]

On 2017/03/19 at 06:09:55, haraken wrote:
> A couple of questions:
> 
> - In terms of binary size, maybe should we consider generalizing this
optimization to other V8 strings created from const char*s?

In terms of binary size, one call isn't particularly big. It's a win for
dictionaries because we typically have several string keys that we always use at
the same time. I'm not sure the optimization generalizes as well to all of the
other users. One of the other big users is the KeepAlive keys (which I optimized
slightly in another CL), which should actually probably be moving to use
V8PrivateProperty (and would want to cache the v8::Private symbols, rather than
the v8::String that are used to retrieve them).

> - In terms of performance, the core problem is that v8AtomicString is slow.
Instead of creating a "string cache" on the binding layer, would it be possible
to try to make v8AtomicString faster?

v8AtomicString with a constant string simply compiles down to a call to
v8::String::NewFromOneByte with v8::NewStringType::kInternalized. The profile
shows time spent in the v8 string table, string hash, etc. While I don't know
that we can't make improvements there, my understanding is that the string table
is already very well-optimized (because it's used whenever a string is
internalized, which IIUC happens for every JS string literal and every string
used a property key).

[haraken, 2017-03-21 02:51:13]

On 2017/03/20 17:45:08, jbroman wrote:
> [cc verwaest, who also suggested caching the internalized strings on the bug]
> 
> On 2017/03/19 at 06:09:55, haraken wrote:
> > A couple of questions:
> > 
> > - In terms of binary size, maybe should we consider generalizing this
> optimization to other V8 strings created from const char*s?
> 
> In terms of binary size, one call isn't particularly big. It's a win for
> dictionaries because we typically have several string keys that we always use
at
> the same time. I'm not sure the optimization generalizes as well to all of the
> other users. One of the other big users is the KeepAlive keys (which I
optimized
> slightly in another CL), which should actually probably be moving to use
> V8PrivateProperty (and would want to cache the v8::Private symbols, rather
than
> the v8::String that are used to retrieve them).
> 
> > - In terms of performance, the core problem is that v8AtomicString is slow.
> Instead of creating a "string cache" on the binding layer, would it be
possible
> to try to make v8AtomicString faster?
> 
> v8AtomicString with a constant string simply compiles down to a call to
> v8::String::NewFromOneByte with v8::NewStringType::kInternalized. The profile
> shows time spent in the v8 string table, string hash, etc. While I don't know
> that we can't make improvements there, my understanding is that the string
table
> is already very well-optimized (because it's used whenever a string is
> internalized, which IIUC happens for every JS string literal and every string
> used a property key).

FWIW, historically we had many caches on the binding side and have removed it
over time. For example, we even had a cache for small integers but removed it by
improving the performance of v8::Integer::New().

I'm just wondering if we should make v8AtomicString faster or introduce a cache
in the binding layer.

[jbroman, 2017-03-22 14:53:21]

On 2017/03/21 at 02:51:13, haraken wrote:
> On 2017/03/20 17:45:08, jbroman wrote:
> > [cc verwaest, who also suggested caching the internalized strings on the
bug]
> > 
> > On 2017/03/19 at 06:09:55, haraken wrote:
> > > A couple of questions:
> > > 
> > > - In terms of binary size, maybe should we consider generalizing this
> > optimization to other V8 strings created from const char*s?
> > 
> > In terms of binary size, one call isn't particularly big. It's a win for
> > dictionaries because we typically have several string keys that we always
use at
> > the same time. I'm not sure the optimization generalizes as well to all of
the
> > other users. One of the other big users is the KeepAlive keys (which I
optimized
> > slightly in another CL), which should actually probably be moving to use
> > V8PrivateProperty (and would want to cache the v8::Private symbols, rather
than
> > the v8::String that are used to retrieve them).
> > 
> > > - In terms of performance, the core problem is that v8AtomicString is
slow.
> > Instead of creating a "string cache" on the binding layer, would it be
possible
> > to try to make v8AtomicString faster?
> > 
> > v8AtomicString with a constant string simply compiles down to a call to
> > v8::String::NewFromOneByte with v8::NewStringType::kInternalized. The
profile
> > shows time spent in the v8 string table, string hash, etc. While I don't
know
> > that we can't make improvements there, my understanding is that the string
table
> > is already very well-optimized (because it's used whenever a string is
> > internalized, which IIUC happens for every JS string literal and every
string
> > used a property key).
> 
> FWIW, historically we had many caches on the binding side and have removed it
over time. For example, we even had a cache for small integers but removed it by
improving the performance of v8::Integer::New().
> 
> I'm just wondering if we should make v8AtomicString faster or introduce a
cache in the binding layer.

I did some exploring. I can make it slightly faster (on my workstation, anyhow
-- this optimization is micro enough that it may vary by platform):
https://codereview.chromium.org/2760413002/
But that only captures ~1/3 of the cost (further work might get it to 1/2;
without larger changes to v8::i::StringTable that are likely to have side
effects I doubt it'll move much further).

WDYT?

[haraken, 2017-03-22 15:05:26]

haraken@chromium.org changed reviewers:
+ jochen@chromium.org

[haraken, 2017-03-22 15:05:26]

+jochen

The question is if we should cache typical V8 strings in the binding side or
not. See comment #20 and #21.

[jochen (gone - plz use gerrit), 2017-03-22 15:09:08]

Maybe v8 and Blink can use the same hash for strings, then we can easily find
corresponding strings on either side

[jbroman, 2017-03-22 15:21:15]

On 2017/03/22 at 15:09:08, jochen wrote:
> Maybe v8 and Blink can use the same hash for strings, then we can easily find
corresponding strings on either side

A couple thoughts there:

1. Only about half the time comes from hashing; the other half comes from
confirming the match. Both can be reduced somewhat with better inlining.

2. V8's string hashing algorithm is seeded by isolate()->heap()->HashSeed(). Am
I right in understanding that it can vary at runtime (though the snapshot might
fix it in practice) to prevent attackers introducing pathological hashtable
behaviour? That would seem to prevent compile-time computation of the hash. And
at runtime, it's not obvious to me whether we should expect Blink's atomic
string table to be any faster than V8's (and we still have to do a lookup on the
V8 side, albeit perhaps a slightly faster one).

[jochen (gone - plz use gerrit), 2017-03-23 15:50:37]

The idea of sharing the hash is that if you already have an AtomicString or an
internalized v8::String, at least you don't need to recompute the hash before
finding the other one

but we should probably just measure what is faster

[jbroman, 2017-03-23 16:00:47]

On 2017/03/23 at 15:50:37, jochen wrote:
> The idea of sharing the hash is that if you already have an AtomicString or an
internalized v8::String, at least you don't need to recompute the hash before
finding the other one
> 
> but we should probably just measure what is faster

Understood, but in this case we start with a string literal. We could maybe get
static WTF::Strings here if we wanted to, but otherwise that seems like an
orthogonal question. Am I misunderstanding?

[jochen (gone - plz use gerrit), 2017-03-24 09:36:13]

On 2017/03/23 at 16:00:47, jbroman wrote:
> On 2017/03/23 at 15:50:37, jochen wrote:
> > The idea of sharing the hash is that if you already have an AtomicString or
an internalized v8::String, at least you don't need to recompute the hash before
finding the other one
> > 
> > but we should probably just measure what is faster
> 
> Understood, but in this case we start with a string literal. We could maybe
get static WTF::Strings here if we wanted to, but otherwise that seems like an
orthogonal question. Am I misunderstanding?

yeah

Overall, I'm deferring to Kentaro here

[haraken, 2017-03-24 12:03:46]

On 2017/03/24 09:36:13, jochen wrote:
> On 2017/03/23 at 16:00:47, jbroman wrote:
> > On 2017/03/23 at 15:50:37, jochen wrote:
> > > The idea of sharing the hash is that if you already have an AtomicString
or
> an internalized v8::String, at least you don't need to recompute the hash
before
> finding the other one
> > > 
> > > but we should probably just measure what is faster
> > 
> > Understood, but in this case we start with a string literal. We could maybe
> get static WTF::Strings here if we wanted to, but otherwise that seems like an
> orthogonal question. Am I misunderstanding?
> 
> yeah
> 
> Overall, I'm deferring to Kentaro here

Hmm.

v8AtomicString is intending to be a cache to speed up a V8 string look-up. If we
have to introduce another cache for V8 strings returned by v8AtomicString, I
guess we're doing something wrong.

What's the slow part of v8AtomicString? If the slow part is the hash
calculation, can we add a use_this_hash parameter to v8AtomicString and pass in
the address of the string literal?

[jbroman, 2017-03-24 13:48:34]

On 2017/03/24 at 12:03:46, haraken wrote:
> On 2017/03/24 09:36:13, jochen wrote:
> > On 2017/03/23 at 16:00:47, jbroman wrote:
> > > On 2017/03/23 at 15:50:37, jochen wrote:
> > > > The idea of sharing the hash is that if you already have an AtomicString
or
> > an internalized v8::String, at least you don't need to recompute the hash
before
> > finding the other one
> > > > 
> > > > but we should probably just measure what is faster
> > > 
> > > Understood, but in this case we start with a string literal. We could
maybe
> > get static WTF::Strings here if we wanted to, but otherwise that seems like
an
> > orthogonal question. Am I misunderstanding?
> > 
> > yeah
> > 
> > Overall, I'm deferring to Kentaro here
> 
> Hmm.
> 
> v8AtomicString is intending to be a cache to speed up a V8 string look-up. If
we have to introduce another cache for V8 strings returned by v8AtomicString, I
guess we're doing something wrong.
> 
> What's the slow part of v8AtomicString?

In my profiles, it was about (as I recall -- I don't have the numbers in front
of me) 40% hash computation, 40% "is this entry a match", 20% misc overhead. I
was able to trim some off that in micro-optimization, but the proportions
remained similar. If we're willing to push devirtualization further into
StringTable than I did, I might be able to make it somewhat faster, but probably
not free.

> If the slow part is the hash calculation, can we add a use_this_hash parameter
to v8AtomicString and pass in the address of the string literal?

Won't that make us do the hash lookup twice? We need the string to actually be
an internalized V8 string, because otherwise we'll internalize it anyway when we
try to do property lookup (LookupIterator calls Factory::InternalizeName to do
so). That implies it has to be in the string table keyed by the correct v8
string hash.

We could have a separate V8-side string table keyed by Blink (or other embedded)
provided keys/hash values, but that seems functionally equivalent to a
Blink-side cache, and I'm not sure I understand why it would be preferable to a
Blink-side cache.

[haraken, 2017-03-24 16:48:31]

On 2017/03/24 13:48:34, jbroman wrote:
> On 2017/03/24 at 12:03:46, haraken wrote:
> > On 2017/03/24 09:36:13, jochen wrote:
> > > On 2017/03/23 at 16:00:47, jbroman wrote:
> > > > On 2017/03/23 at 15:50:37, jochen wrote:
> > > > > The idea of sharing the hash is that if you already have an
AtomicString
> or
> > > an internalized v8::String, at least you don't need to recompute the hash
> before
> > > finding the other one
> > > > > 
> > > > > but we should probably just measure what is faster
> > > > 
> > > > Understood, but in this case we start with a string literal. We could
> maybe
> > > get static WTF::Strings here if we wanted to, but otherwise that seems
like
> an
> > > orthogonal question. Am I misunderstanding?
> > > 
> > > yeah
> > > 
> > > Overall, I'm deferring to Kentaro here
> > 
> > Hmm.
> > 
> > v8AtomicString is intending to be a cache to speed up a V8 string look-up.
If
> we have to introduce another cache for V8 strings returned by v8AtomicString,
I
> guess we're doing something wrong.
> > 
> > What's the slow part of v8AtomicString?
> 
> In my profiles, it was about (as I recall -- I don't have the numbers in front
> of me) 40% hash computation, 40% "is this entry a match", 20% misc overhead. I
> was able to trim some off that in micro-optimization, but the proportions
> remained similar. If we're willing to push devirtualization further into
> StringTable than I did, I might be able to make it somewhat faster, but
probably
> not free.
> 
> > If the slow part is the hash calculation, can we add a use_this_hash
parameter
> to v8AtomicString and pass in the address of the string literal?
> 
> Won't that make us do the hash lookup twice? We need the string to actually be
> an internalized V8 string, because otherwise we'll internalize it anyway when
we
> try to do property lookup (LookupIterator calls Factory::InternalizeName to do
> so). That implies it has to be in the string table keyed by the correct v8
> string hash.

Can we solve the problem by sharing the hashing function between v8 and Blink?

Basically -- I just want to understand what should happen in the ideal state. If
the ideal state is too far from the current state, I'm okay with creating a
cache on the binding side.

I'm sorry that I'm a bit stubborn here. I want to be careful about this change
because:

- Historically we have made similar mistakes in the past. We introduced a cache
for small integers and then removed it by speeding up v8::Integer::New. We
introduced WrapperBoilerplateMap but now want to remove it by speeding up a
wrapper creation on the V8 side.

- In general, V8 bindings is slower than JSC bindings. One of the reasons is
that V8 and Blink are implemented as separate modules with a clear API boundary.
JSC and WebKit can interact with each other in a more intrusive manner. I think
we should consider a way to get V8 and Blink closer to each other and make the
interaction faster (without losing the good API boundary).


> 
> We could have a separate V8-side string table keyed by Blink (or other
embedded)
> provided keys/hash values, but that seems functionally equivalent to a
> Blink-side cache, and I'm not sure I understand why it would be preferable to
a
> Blink-side cache.

[jbroman, 2017-03-24 18:01:44]

On 2017/03/24 at 16:48:31, haraken wrote:
> On 2017/03/24 13:48:34, jbroman wrote:
> > On 2017/03/24 at 12:03:46, haraken wrote:
> > > On 2017/03/24 09:36:13, jochen wrote:
> > > > On 2017/03/23 at 16:00:47, jbroman wrote:
> > > > > On 2017/03/23 at 15:50:37, jochen wrote:
> > > > > > The idea of sharing the hash is that if you already have an
AtomicString
> > or
> > > > an internalized v8::String, at least you don't need to recompute the
hash
> > before
> > > > finding the other one
> > > > > > 
> > > > > > but we should probably just measure what is faster
> > > > > 
> > > > > Understood, but in this case we start with a string literal. We could
> > maybe
> > > > get static WTF::Strings here if we wanted to, but otherwise that seems
like
> > an
> > > > orthogonal question. Am I misunderstanding?
> > > > 
> > > > yeah
> > > > 
> > > > Overall, I'm deferring to Kentaro here
> > > 
> > > Hmm.
> > > 
> > > v8AtomicString is intending to be a cache to speed up a V8 string look-up.
If
> > we have to introduce another cache for V8 strings returned by
v8AtomicString, I
> > guess we're doing something wrong.
> > > 
> > > What's the slow part of v8AtomicString?
> > 
> > In my profiles, it was about (as I recall -- I don't have the numbers in
front
> > of me) 40% hash computation, 40% "is this entry a match", 20% misc overhead.
I
> > was able to trim some off that in micro-optimization, but the proportions
> > remained similar. If we're willing to push devirtualization further into
> > StringTable than I did, I might be able to make it somewhat faster, but
probably
> > not free.
> > 
> > > If the slow part is the hash calculation, can we add a use_this_hash
parameter
> > to v8AtomicString and pass in the address of the string literal?
> > 
> > Won't that make us do the hash lookup twice? We need the string to actually
be
> > an internalized V8 string, because otherwise we'll internalize it anyway
when we
> > try to do property lookup (LookupIterator calls Factory::InternalizeName to
do
> > so). That implies it has to be in the string table keyed by the correct v8
> > string hash.
> 
> Can we solve the problem by sharing the hashing function between v8 and Blink?

Partially. As I indicated above, hashing is only part of the cost. And it's
complicated by V8's hashing function being specialized in a few key ways:
1. It's seeded (which prevents us from computing it at compile time unless we
know the seed -- but avoiding that is usually the point of dynamically seeded
hash functions).
2. It treats "array index-like" and "other" strings different. I think this is
an optimization due to the way that object properties work in V8 (where both
integers and strings can be used as object keys, so array-index-like keys are a
special kind of string that we'd like to be able to construct and hash quickly).

And in this particular case, we don't have either a V8-side or Blink-side hash
value handy (because we're starting with a static string, not a
WTF::AtomicString).

Ideally (at least from a performance standpoint) we would just index into an
array. This CL goes part-way, in that it does a one hash lookup on the Blink
side per dictionary, and then array-indexes from there.

> Basically -- I just want to understand what should happen in the ideal state.
If the ideal state is too far from the current state, I'm okay with creating a
cache on the binding side.
> 
> I'm sorry that I'm a bit stubborn here.

No worries; I understand that you want to get to the right solution here.

> I want to be careful about this change because:
> 
> - Historically we have made similar mistakes in the past. We introduced a
cache for small integers and then removed it by speeding up v8::Integer::New. We
introduced WrapperBoilerplateMap but now want to remove it by speeding up a
wrapper creation on the V8 side.

Fair enough. I don't know the history behind the v8::Integer cache, but that one
does seem somewhat simpler than this (because of V8's SMI representation being
particularly simple).

> - In general, V8 bindings is slower than JSC bindings. One of the reasons is
that V8 and Blink are implemented as separate modules with a clear API boundary.
JSC and WebKit can interact with each other in a more intrusive manner. I think
we should consider a way to get V8 and Blink closer to each other and make the
interaction faster (without losing the good API boundary).

From my perspective, this is rather similar to how V8 does the same thing
internally. The analogous thing in V8 is the isolate's internalized string
roots:
https://cs.chromium.org/chromium/src/v8/src/factory.h?type=cs&l=700

That is, each isolate contains an array of "strong roots": everlasting
strongly-owned handles to objects that live as long as the isolate. Among these
are internalized strings that V8 uses often, like "arguments", "length", and
"byteOffset". These could be cached, but it happens that they are eagerly
initialized in v8::internal::Heap::CreateInitialObjects.

So inside V8 code, I can write things like:

// compiles down to read at a constant offset from the isolate pointer
Handle<String> key = isolate->factory()->length_string();

Blink doesn't have strong roots directly in the v8::internal::Heap object, but
eternal handles are the same thing (except that they are allocated dynamically),
and kept alive in the same way (Heap::IterateStrongRoots).

From a "making them more similar" perspective, the most similar thing we could
do is have a fixed array of eternal dictionary keys directly by
V8PerIsolateData, and initialize them when the isolate boots (which avoids
having to check for initialization on use, at the cost of doing more allocations
at startup).

> > We could have a separate V8-side string table keyed by Blink (or other
embedded)
> > provided keys/hash values, but that seems functionally equivalent to a
> > Blink-side cache, and I'm not sure I understand why it would be preferable
to a
> > Blink-side cache.

[haraken, 2017-03-27 05:13:25]

Thanks for the detailed explanation. I'm convinced :)

The final question: This CL is creating a map from key to Vector<Eternal> but
would it be more general to create a map from key to one Eternal? Then we'll be
able to use the cache for things other than dictionary keys. (We can probably
expose v8AtomicStringFast() on top of the cache.)

[jbroman, 2017-03-29 19:23:01]

On 2017/03/27 at 05:13:25, haraken wrote:
> Thanks for the detailed explanation. I'm convinced :)
> 
> The final question: This CL is creating a map from key to Vector<Eternal> but
would it be more general to create a map from key to one Eternal? Then we'll be
able to use the cache for things other than dictionary keys. (We can probably
expose v8AtomicStringFast() on top of the cache.)

I've implemented this locally to compare. It turns out that you get about 50-70%
of the win, but the rest seems to arise from the fact that you only have to do
one lookup for the whole dictionary for the current approach (and thereafter can
just keep the result in a register). DOMMatrixInit is an extreme case (with 23
keys and little other work), of course.

From an APK size perspective, that option saves about 10k instead of 20k
(basically because the per-key lookup code is a function call rather than an
offset-from-register).

It happens that for dictionaries we always use the same set of keys at once,
which enables us to edge out these wins (naturally, this would matter less if we
used a flat array), but I acknowledge that it's slightly less clean as
abstractions go.

The major users of v8AtomicString with a string literal today are:
- dictionary keys (i.e. what this patch is considering)
- KeepAlive (probably move from V8HiddenValue to V8PrivateProperty anyhow)
- CachedAttribute (probably move from V8HiddenValue to V8PrivateProperty anyhow)
- [Replaceable] (very few uses)
- toV8Sequence (uses "length" -- if this turned out to be hot, it'd be
reasonable to special-case this)
- origin-safe methods (probably move from V8HiddenValue to V8PrivateProperty
anyhow)
- unscopeables (only happens when creating the prototype)

WDYT?

[Toon Verwaest, 2017-03-30 08:46:32]

verwaest@chromium.org changed reviewers:
+ verwaest@chromium.org

[Toon Verwaest, 2017-03-30 08:46:32]

The approach seems good to me.

Note that the idea isn't to just cache the strings in the long run, but possibly
also introduce API inline caches that allow us to speedup property lookup as
well. These eternal handles are the perfect place to store such per-isolate API 
ICs. Just sharing the string table between V8 and Blink or related tweaks
wouldn't bring us closer towards that goal.

[haraken, 2017-03-30 14:38:40]

On 2017/03/29 19:23:01, jbroman wrote:
> On 2017/03/27 at 05:13:25, haraken wrote:
> > Thanks for the detailed explanation. I'm convinced :)
> > 
> > The final question: This CL is creating a map from key to Vector<Eternal>
but
> would it be more general to create a map from key to one Eternal? Then we'll
be
> able to use the cache for things other than dictionary keys. (We can probably
> expose v8AtomicStringFast() on top of the cache.)
> 
> I've implemented this locally to compare. It turns out that you get about
50-70%
> of the win, but the rest seems to arise from the fact that you only have to do
> one lookup for the whole dictionary for the current approach (and thereafter
can
> just keep the result in a register). DOMMatrixInit is an extreme case (with 23
> keys and little other work), of course.
> 
> From an APK size perspective, that option saves about 10k instead of 20k
> (basically because the per-key lookup code is a function call rather than an
> offset-from-register).
> 
> It happens that for dictionaries we always use the same set of keys at once,
> which enables us to edge out these wins (naturally, this would matter less if
we
> used a flat array), but I acknowledge that it's slightly less clean as
> abstractions go.
> 
> The major users of v8AtomicString with a string literal today are:
> - dictionary keys (i.e. what this patch is considering)
> - KeepAlive (probably move from V8HiddenValue to V8PrivateProperty anyhow)
> - CachedAttribute (probably move from V8HiddenValue to V8PrivateProperty
anyhow)
> - [Replaceable] (very few uses)
> - toV8Sequence (uses "length" -- if this turned out to be hot, it'd be
> reasonable to special-case this)
> - origin-safe methods (probably move from V8HiddenValue to V8PrivateProperty
> anyhow)
> - unscopeables (only happens when creating the prototype)
> 
> WDYT?

Thanks for the investigation! LGTM :D

I might want to rename findOrCreateKeys() to cachedEternalNameVector() or
something to clarify that it's a vector version of the Eternal Name caching. (We
could introduce cachedEternalName() in the future if it's useful for cases where
we want to cache one eternal name.)

[jbroman, 2017-03-30 15:47:19]

On 2017/03/30 at 14:38:40, haraken wrote:
> On 2017/03/29 19:23:01, jbroman wrote:
> > On 2017/03/27 at 05:13:25, haraken wrote:
> > > Thanks for the detailed explanation. I'm convinced :)
> > > 
> > > The final question: This CL is creating a map from key to Vector<Eternal>
but
> > would it be more general to create a map from key to one Eternal? Then we'll
be
> > able to use the cache for things other than dictionary keys. (We can
probably
> > expose v8AtomicStringFast() on top of the cache.)
> > 
> > I've implemented this locally to compare. It turns out that you get about
50-70%
> > of the win, but the rest seems to arise from the fact that you only have to
do
> > one lookup for the whole dictionary for the current approach (and thereafter
can
> > just keep the result in a register). DOMMatrixInit is an extreme case (with
23
> > keys and little other work), of course.
> > 
> > From an APK size perspective, that option saves about 10k instead of 20k
> > (basically because the per-key lookup code is a function call rather than an
> > offset-from-register).
> > 
> > It happens that for dictionaries we always use the same set of keys at once,
> > which enables us to edge out these wins (naturally, this would matter less
if we
> > used a flat array), but I acknowledge that it's slightly less clean as
> > abstractions go.
> > 
> > The major users of v8AtomicString with a string literal today are:
> > - dictionary keys (i.e. what this patch is considering)
> > - KeepAlive (probably move from V8HiddenValue to V8PrivateProperty anyhow)
> > - CachedAttribute (probably move from V8HiddenValue to V8PrivateProperty
anyhow)
> > - [Replaceable] (very few uses)
> > - toV8Sequence (uses "length" -- if this turned out to be hot, it'd be
> > reasonable to special-case this)
> > - origin-safe methods (probably move from V8HiddenValue to V8PrivateProperty
> > anyhow)
> > - unscopeables (only happens when creating the prototype)
> > 
> > WDYT?
> 
> Thanks for the investigation! LGTM :D
> 
> I might want to rename findOrCreateKeys() to cachedEternalNameVector() or
something to clarify that it's a vector version of the Eternal Name caching. (We
could introduce cachedEternalName() in the future if it's useful for cases where
we want to cache one eternal name.)

Renamed to "findOrCreateEternalNameCache". (In the future we could either
overload it, or call it "findOrCreateEternalName" or something.)

[jbroman, 2017-03-30 15:48:14]

The CQ bit was checked by jbroman@chromium.org

[jbroman, 2017-03-30 15:48:14]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2751263002/#ps80001
(title: "findOrCreateEternalNameCache")

[commit-bot: I haz the power, 2017-03-30 15:48:58]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 18:11:56]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490888894206180,
"parent_rev": "4cd903e0450bf609707bc83c5d1f4b415bd51fbc", "commit_rev":
"53d467c4d4be2da1882d9c17a4384906a7f6d7e2"}

[commit-bot: I haz the power, 2017-03-30 18:12:43]

Description was changed from

==========
[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
==========

to

==========
[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/+/53d467c4d4be2da1882d9c17a438...
==========

[commit-bot: I haz the power, 2017-03-30 18:12:45]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/53d467c4d4be2da1882d9c17a438...