Reland "Limit the number of transitions allowed per hidden class."

Reland "Limit the number of transitions allowed per hidden class."

BUG=chromium:427813
LOG=N
R=verwaest@chromium.org

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

[Igor Sheludko, 2014-10-27 14:19:42]

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

[Igor Sheludko, 2014-10-27 14:19:43]

PTAL

https://codereview.chromium.org/676393002/diff/20001/src/transitions.cc
File src/transitions.cc (right):

https://codereview.chromium.org/676393002/diff/20001/src/transitions.cc#newco...
src/transitions.cc:156: if (insertion_index != kNotFound) {
Without this piece we could end up having duplicate entries for the same name.

[Igor Sheludko, 2014-10-28 08:03:21]

ishell@chromium.org changed reviewers:
+ mckev@amazon.com

[Igor Sheludko, 2014-10-28 09:21:42]

Patchset #2 (id:20001) has been deleted

[mckev, 2014-10-28 17:31:18]

Thanks for relanding this patch!

On 2014/10/27 14:19:43, Igor Sheludko wrote:
> https://codereview.chromium.org/676393002/diff/20001/src/transitions.cc
> File src/transitions.cc (right):
> 
>
https://codereview.chromium.org/676393002/diff/20001/src/transitions.cc#newco...
> src/transitions.cc:156: if (insertion_index != kNotFound) {
> Without this piece we could end up having duplicate entries for the same name.

Just out of curiosity, when does this occur?  If we already have the key,
I thought we could just replace that entry in the existing array.

Two other questions / comments (for clarification):

1) How do we end up with
`new_nof <= map->transitions()->number_of_transitions_storage()` being false but
`insertion_index != kNotFound` is true?

2) One concern might be that we're trimming the transitions array without
updating
`number_of_transitions` somewhere, which would cause `number_of_transitions` to
be erroneously greater than `number_of_transitions_storage`.

[Igor Sheludko, 2014-10-30 16:15:26]

On 2014/10/28 17:31:18, mckev wrote:
> Thanks for relanding this patch!
> 
> On 2014/10/27 14:19:43, Igor Sheludko wrote:
> > https://codereview.chromium.org/676393002/diff/20001/src/transitions.cc
> > File src/transitions.cc (right):
> > 
> >
>
https://codereview.chromium.org/676393002/diff/20001/src/transitions.cc#newco...
> > src/transitions.cc:156: if (insertion_index != kNotFound) {
> > Without this piece we could end up having duplicate entries for the same
name.
> 
> Just out of curiosity, when does this occur?  If we already have the key,
> I thought we could just replace that entry in the existing array.
> 
> Two other questions / comments (for clarification):
> 
> 1) How do we end up with
> `new_nof <= map->transitions()->number_of_transitions_storage()` being false
but
> `insertion_index != kNotFound` is true?
> 
> 2) One concern might be that we're trimming the transitions array without
> updating
> `number_of_transitions` somewhere, which would cause `number_of_transitions`
to
> be erroneously greater than `number_of_transitions_storage`.

1) Yes, indeed. I'm sorry.
2) Yes, but it's not clear so far where it actually happens.

[Igor Sheludko, 2014-10-30 16:38:37]

On 2014/10/30 16:15:26, Igor Sheludko wrote:
> On 2014/10/28 17:31:18, mckev wrote:
> > Thanks for relanding this patch!
> > 
> > On 2014/10/27 14:19:43, Igor Sheludko wrote:
> > > https://codereview.chromium.org/676393002/diff/20001/src/transitions.cc
> > > File src/transitions.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/676393002/diff/20001/src/transitions.cc#newco...
> > > src/transitions.cc:156: if (insertion_index != kNotFound) {
> > > Without this piece we could end up having duplicate entries for the same
> name.
> > 
> > Just out of curiosity, when does this occur?  If we already have the key,
> > I thought we could just replace that entry in the existing array.
> > 
> > Two other questions / comments (for clarification):
> > 
> > 1) How do we end up with
> > `new_nof <= map->transitions()->number_of_transitions_storage()` being false
> but
> > `insertion_index != kNotFound` is true?
> > 
> > 2) One concern might be that we're trimming the transitions array without
> > updating
> > `number_of_transitions` somewhere, which would cause `number_of_transitions`
> to
> > be erroneously greater than `number_of_transitions_storage`.
> 
> 1) Yes, indeed. I'm sorry.
> 2) Yes, but it's not clear so far where it actually happens.

I just realized that the number_of_elements() of broken arrays were bigger than
TransitionArray::kMaxNumberOfTransitions.

[Igor Sheludko, 2014-10-31 08:16:04]

Patchset #2 (id:40001) has been deleted

[Toon Verwaest, 2014-11-03 16:24:45]

lgtm

[Igor Sheludko, 2014-11-03 16:46:05]

Committed patchset #2 (id:60001) manually as 25082 (presubmit successful).

[mckev, 2014-11-03 17:17:29]

On 2014/11/03 16:46:05, Igor Sheludko wrote:
> Committed patchset #2 (id:60001) manually as 25082 (presubmit successful).

Thanks again for your help re-landing this!