Allocation type info advice consumed in bailout path leads to assert failure.

Allocation type info advice consumed in bailout path leads to assert failure.

If the runtime is taken for a constructor like "new Array(100000)", where
allocation site info already led to an elements kind of DOUBLE, then the runtime
would fail to transition the array to dictionary mode. Better to recognize this
case and avoid wasting time by following the advice. Furthermore, it offers a
way to recognize that the array should be in dictionary mode (though a future
checkin will capitalize on that).

BUG=
R=danno@chromium.org

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

[mvstanton, 2013-06-04 15:43:16]

Hi Danno, here is the bugfix I referred to earlier today. I also added a test.
Without the fix, the test fails with an assert under
JSObject::GetElementsCapacityAndUsage() in the bailout.

[danno, 2013-06-04 16:29:46]

lgtm with comments

https://codereview.chromium.org/15993012/diff/1/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/15993012/diff/1/src/builtins.cc#newcode212
src/builtins.cc:212: bool ignore_advice = false;
ignore_type_feedback is probably more accurate.

https://codereview.chromium.org/15993012/diff/1/src/builtins.cc#newcode217
src/builtins.cc:217: holey = (value > 0 && value <
JSObject::kInitialMaxFastElementArray);
Don't you mean >= 0? Or is the distinction important?

[mvstanton, 2013-06-05 08:47:07]

Thanks for the comments Danno, I've addressed, and I noticed another issue, see
comments.
--Michael

https://codereview.chromium.org/15993012/diff/1/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/15993012/diff/1/src/builtins.cc#newcode212
src/builtins.cc:212: bool ignore_advice = false;
On 2013/06/04 16:29:47, danno wrote:
> ignore_type_feedback is probably more accurate.

Done.

https://codereview.chromium.org/15993012/diff/1/src/builtins.cc#newcode217
src/builtins.cc:217: holey = (value > 0 && value <
JSObject::kInitialMaxFastElementArray);
On 2013/06/04 16:29:47, danno wrote:
> Don't you mean >= 0? Or is the distinction important?

I think it's important because if someone calls new Array(0), they should get a
packed array instead of a holey one.

But your comment brought to light a bug in this new code. I would choose to
ignore type feedback if the value == 0, which is wrong. I've prepped a more
careful initialization of ignore_type_feedback.

[danno, 2013-06-05 11:31:13]

lgtm with the comment

https://codereview.chromium.org/15993012/diff/4001/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/15993012/diff/4001/src/builtins.cc#newcode212
src/builtins.cc:212: bool ignore_type_feedback = false;
Last comment: I think the logic makes a little more sense if you invert the
sense of this variable, i.e. call it "can_use_type_feedback". That makes the
later tests a little more readable.

[mvstanton, 2013-06-06 09:23:33]

Awesome, thanks!

https://codereview.chromium.org/15993012/diff/4001/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/15993012/diff/4001/src/builtins.cc#newcode212
src/builtins.cc:212: bool ignore_type_feedback = false;
On 2013/06/05 11:31:13, danno wrote:
> Last comment: I think the logic makes a little more sense if you invert the
> sense of this variable, i.e. call it "can_use_type_feedback". That makes the
> later tests a little more readable.

Done.

[mvstanton, 2013-06-06 09:26:36]

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