Prevent inline constructor generation when duplicate properties are present i...

Prevent inline constructor generation when duplicate properties are present in the constructor.

Currenly the constructor like this:
function f() {
  this.a = 0;
  this.a = 1;
  this.a = 2;
}
creates a map with duplicate desciptors which is bad in many ways.



Committed: http://code.google.com/p/v8/source/detail?r=5476

[Mads Ager (chromium), 2010-09-15 14:42:25]

Drive-by: when you agree on the patch, please merge to 2.2 and 2.3 branches. :)

[Søren Thygesen Gjesse, 2010-09-16 06:43:41]

LGTM, good catch

Whether you want to move the checking to
SharedFunctionInfo::SetThisPropertyAssignmentsInfo is up to you.

http://codereview.chromium.org/3434004/diff/1/2
File src/heap.cc (right):

http://codereview.chromium.org/3434004/diff/1/2#newcode2717
src/heap.cc:2717: // guarantee the uniqueness of property names (it would have
required
We could do the sorting and checking for duplicates in
SharedFunctionInfo::SetThisPropertyAssignmentsInfo instead. If there are
duplicates then disable there.

There we could also weed out all but the last assignment to a property and
support the case of multiple assignments to a property, but it is probably
(hopefuly) a rare case.

If the sorting is done in SharedFunctionInfo::SetThisPropertyAssignmentsInfo we
should not need to sort here.

[Vladislav Kaznacheev, 2010-09-16 10:50:21]

http://codereview.chromium.org/3434004/diff/1/2
File src/heap.cc (right):

http://codereview.chromium.org/3434004/diff/1/2#newcode2717
src/heap.cc:2717: // guarantee the uniqueness of property names (it would have
required
On 2010/09/16 06:43:41, Søren Gjesse wrote:
> We could do the sorting and checking for duplicates in
> SharedFunctionInfo::SetThisPropertyAssignmentsInfo instead. If there are
> duplicates then disable there.

I have considered this and decided against it. If we do the sorting/checking at
parse time we will have to do it even if the function is never called as a
constructor. So this would probably be slower, and require more code (separate
sorting function).

> There we could also weed out all but the last assignment to a property and
> support the case of multiple assignments to a property, but it is probably
> (hopefuly) a rare case.

This is extremely rare case. It never occurs in any of our tests (the is an
assert for IsSortedNoDuplicate, so we would have noticed).

> If the sorting is done in SharedFunctionInfo::SetThisPropertyAssignmentsInfo
we
> should not need to sort here.