Hydrogenized KeyedLoadGeneric stub: when probing the KeyedLookupCache fails, call the runtime, don'…

Hydrogenized KeyedLoadGeneric stub: when probing the KeyedLookupCache fails, call the runtime, don't stub-fail.

R=danno@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=22254

[Jakob Kummerow, 2014-07-03 16:19:43]

PTAL.

[danno, 2014-07-07 07:59:06]

https://codereview.chromium.org/367343002/diff/1/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):

https://codereview.chromium.org/367343002/diff/1/src/code-stubs-hydrogen.cc#n...
src/code-stubs-hydrogen.cc:1662: IfBuilder* lookup_if = new IfBuilder(this);
Shouldn't you just Zone allocate this?

void* buffer = zone->New(sizeof(IfBuilder));
IfBuidler* lookup_if = new (buffer) IfBuilder(this)?

[Jakob Kummerow, 2014-07-07 11:52:10]

https://codereview.chromium.org/367343002/diff/1/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):

https://codereview.chromium.org/367343002/diff/1/src/code-stubs-hydrogen.cc#n...
src/code-stubs-hydrogen.cc:1662: IfBuilder* lookup_if = new IfBuilder(this);
On 2014/07/07 07:59:06, danno wrote:
> Shouldn't you just Zone allocate this?
> 
> void* buffer = zone->New(sizeof(IfBuilder));
> IfBuidler* lookup_if = new (buffer) IfBuilder(this)?

I'm not sure. In what way would that be better? We never zone-allocate
IfBuilders. And I don't quite see why we should; they're very short-lived
(whereas the graph builder's zone lives "forever"); and we don't need that many
of them (just kEntriesPerBucket == 4).
But if you prefer zone allocation, I can certainly zone-allocate them.

[danno, 2014-07-07 12:08:45]

lgtm with comment

https://codereview.chromium.org/367343002/diff/1/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):

https://codereview.chromium.org/367343002/diff/1/src/code-stubs-hydrogen.cc#n...
src/code-stubs-hydrogen.cc:1662: IfBuilder* lookup_if = new IfBuilder(this);
On 2014/07/07 11:52:10, Jakob wrote:
> On 2014/07/07 07:59:06, danno wrote:
> > Shouldn't you just Zone allocate this?
> > 
> > void* buffer = zone->New(sizeof(IfBuilder));
> > IfBuidler* lookup_if = new (buffer) IfBuilder(this)?
> 
> I'm not sure. In what way would that be better? We never zone-allocate
> IfBuilders. And I don't quite see why we should; they're very short-lived
> (whereas the graph builder's zone lives "forever"); and we don't need that
many
> of them (just kEntriesPerBucket == 4).
> But if you prefer zone allocation, I can certainly zone-allocate them.

Well, since we generally avoid allocating from the C++ heap, I personally think
the Zone allocation would be better, but it's your call.

[Jakob Kummerow, 2014-07-07 13:08:28]

OK, here's a version that stack-allocates those IfBuilders. Do you like it?

[danno, 2014-07-07 14:59:52]

I like this, but it would be great to have some asserts that made sure that
accidentally calling the default constructor tells you what you're doing wrong:

IfBuilder builder;  // Forgot (this)
builder.IfXXXX    // What exactly happens here?

[Jakob Kummerow, 2014-07-07 15:11:03]

> IfBuilder builder;  // Forgot (this)
> builder.IfXXXX    // What exactly happens here?

You'll get a NULL deref, which should be fairly obvious... but an explicit
ASSERT is even more obvious. See patch set 3 (blame clang-format for the
whitespace changes).

[Jakob Kummerow, 2014-07-07 16:24:59]

Committed patchset #3 manually as r22254 (presubmit successful).