v8:3539 - hold constructor feedback in weak cells

v8:3539 - hold constructor feedback in weak cells

BUG=v8:3539
R=verwaest@chromium.org
LOG=N

Committed: https://crrev.com/b134ae74b5fe4750588cef81a06b6fabd2507409
Cr-Commit-Position: refs/heads/master@{#27581}

[mvstanton, 2015-03-25 09:54:58]

Patchset #4 (id:60001) has been deleted

[mvstanton, 2015-03-25 09:55:05]

Patchset #3 (id:40001) has been deleted

[mvstanton, 2015-03-25 09:55:12]

Patchset #2 (id:20001) has been deleted

[mvstanton, 2015-03-25 09:55:23]

Patchset #1 (id:1) has been deleted

[mvstanton, 2015-03-25 10:04:24]

Hi Toon,
Here is the introduction of weak cells to feedback slots. Thanks for the look!
--Michael

[Toon Verwaest, 2015-03-25 15:25:52]

lgtm with minor comments

https://codereview.chromium.org/1029093002/diff/100001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

https://codereview.chromium.org/1029093002/diff/100001/src/arm/code-stubs-arm...
src/arm/code-stubs-arm.cc:2426: __ CompareRoot(r4,
Heap::kmegamorphic_symbolRootIndex);
Checking this second might be faster...

https://codereview.chromium.org/1029093002/diff/100001/src/type-feedback-vect...
File src/type-feedback-vector.cc (right):

https://codereview.chromium.org/1029093002/diff/100001/src/type-feedback-vect...
src/type-feedback-vector.cc:167: // if (instance_type != WEAK_CELL_TYPE ||
force_clear) {
Remove debugging-comment

[mvstanton, 2015-03-26 15:28:53]

Thanks Toon, I've addressed comments and also added static asserts around the
assumption that I can safely look into the weak cell value without truly knowing
yet if I have a weak cell or not. Landing when MIPS ports are ready...

https://codereview.chromium.org/1029093002/diff/100001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

https://codereview.chromium.org/1029093002/diff/100001/src/arm/code-stubs-arm...
src/arm/code-stubs-arm.cc:2426: __ CompareRoot(r4,
Heap::kmegamorphic_symbolRootIndex);
On 2015/03/25 15:25:51, Toon Verwaest wrote:
> Checking this second might be faster...

Good idea, done.

https://codereview.chromium.org/1029093002/diff/100001/src/type-feedback-vect...
File src/type-feedback-vector.cc (right):

https://codereview.chromium.org/1029093002/diff/100001/src/type-feedback-vect...
src/type-feedback-vector.cc:167: // if (instance_type != WEAK_CELL_TYPE ||
force_clear) {
On 2015/03/25 15:25:51, Toon Verwaest wrote:
> Remove debugging-comment

Done.

[mvstanton, 2015-04-02 09:14:59]

The CQ bit was checked by mvstanton@chromium.org

[mvstanton, 2015-04-02 09:15:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
verwaest@chromium.org
Link to the patchset: https://codereview.chromium.org/1029093002/#ps140001
(title: "REBASE.")

[commit-bot: I haz the power, 2015-04-02 09:15:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1029093002/140001

[commit-bot: I haz the power, 2015-04-02 09:39:37]

Committed patchset #4 (id:140001)

[commit-bot: I haz the power, 2015-04-02 09:39:46]

Patchset 4 (id:??) landed as
https://crrev.com/b134ae74b5fe4750588cef81a06b6fabd2507409
Cr-Commit-Position: refs/heads/master@{#27581}

[Erik Corry, 2015-04-02 12:54:10]

erik.corry@gmail.com changed reviewers:
+ erik.corry@gmail.com

[Erik Corry, 2015-04-02 12:54:11]

I'm a bit late here, but:

https://codereview.chromium.org/1029093002/diff/140001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

https://codereview.chromium.org/1029093002/diff/140001/src/arm/code-stubs-arm...
src/arm/code-stubs-arm.cc:2377: // Arguments register must be smi-tagged to call
out.
Comment clarification propopsal:
Arguments register -> Number-of-arguments register

https://codereview.chromium.org/1029093002/diff/140001/src/arm/code-stubs-arm...
src/arm/code-stubs-arm.cc:2415: __ CompareRoot(r4,
Heap::kmegamorphic_symbolRootIndex);
We just treated r4 as a weak cell (3 lines up), now we are comparing it with a
symbol. Does that mean we just loaded some random field from a symbol and
compared it with a function?

https://codereview.chromium.org/1029093002/diff/140001/src/arm/code-stubs-arm...
src/arm/code-stubs-arm.cc:2417: __ ldr(feedback_map, FieldMemOperand(r4, 0));
HeapObject::kMapOffset would be better than 0

https://codereview.chromium.org/1029093002/diff/140001/src/arm/code-stubs-arm...
src/arm/code-stubs-arm.cc:2421: // If r1 is not equal to the weak cell value,
and the weak cell value is
Comment clarification suggestion:
r1 -> function

https://codereview.chromium.org/1029093002/diff/140001/src/arm/code-stubs-arm...
src/arm/code-stubs-arm.cc:2431: // AllocationSite. Do a map check on the object
in ecx.
ecx? :-)

[mvstanton, 2015-04-02 14:00:38]

Hi Erik, I'll address the issues you raised with a follow-on CL for you to
review, thanks for the close look. But I'll go ahead and reply to the most
serious issue inline now, thanks again,
--Michael

https://codereview.chromium.org/1029093002/diff/140001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

https://codereview.chromium.org/1029093002/diff/140001/src/arm/code-stubs-arm...
src/arm/code-stubs-arm.cc:2415: __ CompareRoot(r4,
Heap::kmegamorphic_symbolRootIndex);
On 2015/04/02 12:54:11, Erik Corry wrote:
> We just treated r4 as a weak cell (3 lines up), now we are comparing it with a
> symbol. Does that mean we just loaded some random field from a symbol and
> compared it with a function?

Correct, but it's not random. It's a performance optimization to tolerate
ambiguity in what we've loaded and only discover whether it's a WeakCell or
Symbol later. It's safe to examine field offset +kPointerSize, and the continued
safety of this optimization is protected with static asserts in
type-feedback-vector.h (also in this CL).