Monomorphic prototype failures should be reserved for already-seen keys.

Monomorphic prototype failures should be reserved for already-seen keys.

We incorrectly mark a KeyedStoreIC miss as a monomorphic prototype
failure even though it's the first time a particular (string) key has
been seen.

BUG=358088
R=verwaest@chromium.org
LOG=N

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

[mvstanton, 2014-03-31 12:27:52]

Hi Toon, here is the fix we discussed, have a look, thanks!
--Michael

[Toon Verwaest, 2014-03-31 12:34:44]

https://codereview.chromium.org/219313002/diff/1/src/ic.cc
File src/ic.cc (right):

https://codereview.chromium.org/219313002/diff/1/src/ic.cc#newcode1187
src/ic.cc:1187: if (!ic->IsNameCompatibleWithMonomorphicPrototypeFailure(name))
return false;
This is not exactly the same...
The name only has to be compatible for the MarkMonomorphicPrototypeFailure(name)
below. That's why I suggested (and still suggest) moving it inside of
MarkMonomorphicPrototypeFailure(name), so that we don't need to explicitly guard
it at every use of MarkMonomorphicPrototypeFailure(name) (and asserting inside
of the method).

[mvstanton, 2014-03-31 13:30:56]

Hello, PTAL, thanks,
--Michael

https://codereview.chromium.org/219313002/diff/1/src/ic.cc
File src/ic.cc (right):

https://codereview.chromium.org/219313002/diff/1/src/ic.cc#newcode1187
src/ic.cc:1187: if (!ic->IsNameCompatibleWithMonomorphicPrototypeFailure(name))
return false;
On 2014/03/31 12:34:44, Toon Verwaest wrote:
> This is not exactly the same...
> The name only has to be compatible for the
MarkMonomorphicPrototypeFailure(name)
> below. That's why I suggested (and still suggest) moving it inside of
> MarkMonomorphicPrototypeFailure(name), so that we don't need to explicitly
guard
> it at every use of MarkMonomorphicPrototypeFailure(name) (and asserting inside
> of the method).

I ran into an issue with that idea. TryRemoiveInvalidPrototypeDependentStub()
carries out a lot of actions and is better served with a guard against going
forward if the name isn't found in the stub, hence the two-part nature of the
fix.

What I've done now is to

1) change MarkMonomorphicPrototypeFailure to TryMarkMonomorphicPrototypeFailure,
so this code site doesn't need to call
IsNameCompatibleWithMonomorphicPrototypeFailure().
2) In TryRemoiveInvalidPrototypeDependentStub(), an explicit call to
IsNameCompatibleWithMonomorphicPrototypeFailure is made.
3) No more dubious asserts.

[Toon Verwaest, 2014-03-31 15:15:53]

lgtm with nit

https://codereview.chromium.org/219313002/diff/40001/src/ic.cc
File src/ic.cc (right):

https://codereview.chromium.org/219313002/diff/40001/src/ic.cc#newcode362
src/ic.cc:362: receiver, Handle<String>::cast(name))) {
&& TryMarkMono...

[mvstanton, 2014-04-01 14:14:18]

Right on, thank you for the review, best,
--Michael

https://codereview.chromium.org/219313002/diff/40001/src/ic.cc
File src/ic.cc (right):

https://codereview.chromium.org/219313002/diff/40001/src/ic.cc#newcode362
src/ic.cc:362: receiver, Handle<String>::cast(name))) {
On 2014/03/31 15:15:54, Toon Verwaest wrote:
> && TryMarkMono...

Done.

[mvstanton, 2014-04-01 14:17:02]

Committed patchset #4 manually as r20407 (presubmit successful).