Issue 9663002: Use CopyElements for SetFastDoubleElementsCapacityAndLength

Issue 9663002: Use CopyElements for SetFastDoubleElementsCapacityAndLength (Closed)

Created:
8 years, 9 months ago by danno

Modified:
8 years, 9 months ago

Reviewers:
Michael Starzinger, Jakob Kummerow

CC:
v8-dev

Base URL:
https://v8.googlecode.com/svn/branches/bleeding_edge

Visibility:
Public.

More Reviews

Description

Use CopyElements for SetFastDoubleElementsCapacityAndLength Committed: https://code.google.com/p/v8/source/detail?r=11070

Patch Set 3 : Review feedback #

Total comments: 8

Patch Set 4 : Review feedback #

Created: 8 years, 9 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+242 lines, -190 lines)			Patch
M	src/builtins.cc	View	1 2 3	6 chunks	+7 lines, -13 lines	0 comments	Download
M	src/elements.h	View	1	3 chunks	+16 lines, -4 lines	0 comments	Download
M	src/elements.cc	View	1 2	8 chunks	+205 lines, -80 lines	0 comments	Download
M	src/objects.h	View	1 2 3	1 chunk	+0 lines, -4 lines	0 comments	Download
M	src/objects.cc	View	1 2 3	3 chunks	+13 lines, -30 lines	0 comments	Download
M	src/objects-inl.h	View	1 2 3	1 chunk	+0 lines, -59 lines	0 comments	Download
M	src/runtime.cc	View	1 2 3	1 chunk	+1 line, -0 lines	0 comments	Download

Messages

Total messages: 6 (0 generated)

Expand Messages | Collapse Messages

Jakob Kummerow

http://codereview.chromium.org/9663002/diff/2001/src/builtins.cc File src/builtins.cc (left): http://codereview.chromium.org/9663002/diff/2001/src/builtins.cc#oldcode511 src/builtins.cc:511: AssertNoAllocation no_gc; Did you delete this line on purpose? ...

8 years, 9 months ago (2012-03-12 15:33:28 UTC) #3

danno

Feedback addressed http://codereview.chromium.org/9663002/diff/2001/src/builtins.cc File src/builtins.cc (left): http://codereview.chromium.org/9663002/diff/2001/src/builtins.cc#oldcode511 src/builtins.cc:511: AssertNoAllocation no_gc; The AssertNoAllocation doesn't help here, ...

8 years, 9 months ago (2012-03-12 20:35:03 UTC) #4

Feedback addressed

http://codereview.chromium.org/9663002/diff/2001/src/builtins.cc
File src/builtins.cc (left):

http://codereview.chromium.org/9663002/diff/2001/src/builtins.cc#oldcode511
src/builtins.cc:511: AssertNoAllocation no_gc;
The AssertNoAllocation doesn't help here, and actually it's misleading (the code
here is wrong as it was). There can't be an allocation between the time that
new_elms is allocated and the copy happens. But the AssertNoAllocation object
doesn't guarantee that, it only guarantees that there is no allocation since the
no allocation object is created. I talked about this with Starzinger, and we
agreed that it's misleading to suggest that there is some sort of protection
through stack-allocated protection objects when there isn't. In other new code,
the WriteBarrierMode is explicit. Until there is a replacement for
AssertNoAllocation that actually is bound tighter to the allocation that it's
trying to protect (that's a bigger chunk of work), the explicit mode at least
forces the caller to think about what is safe and what is not. 

On 2012/03/12 15:33:28, Jakob wrote:
> Did you delete this line on purpose? AFAICS we don't allocate anything during
> the call, so we might want to keep the assertion.

http://codereview.chromium.org/9663002/diff/2001/src/builtins.cc#oldcode976
src/builtins.cc:976: AssertNoAllocation no_gc;
See above.
On 2012/03/12 15:33:28, Jakob wrote:
> Same question here.

http://codereview.chromium.org/9663002/diff/2001/src/elements.cc
File src/elements.cc (right):

http://codereview.chromium.org/9663002/diff/2001/src/elements.cc#newcode151
src/elements.cc:151: #if DEBUG
On 2012/03/12 15:33:28, Jakob wrote:
> s/#if/#ifdef/
> 
> Again below.

Done.

http://codereview.chromium.org/9663002/diff/2001/src/elements.cc#newcode157
src/elements.cc:157: ASSERT(to->get(i) == hole);
On 2012/03/12 15:33:28, Jakob wrote:
> ASSERT(to->is_the_hole(i))?

Done.

http://codereview.chromium.org/9663002/diff/2001/src/elements.cc#newcode162
src/elements.cc:162: ASSERT(((copy_size + static_cast<int>(to_start)) <=
to->length() &&
On 2012/03/12 15:33:28, Jakob wrote:
> nit: you can remove at least one pair of parentheses.

Done.

http://codereview.chromium.org/9663002/diff/2001/src/elements.cc#newcode202
src/elements.cc:202: ASSERT(to->get(i) == hole);
On 2012/03/12 15:33:28, Jakob wrote:
> ASSERT(to->is_the_hole(i))?

Done.

http://codereview.chromium.org/9663002/diff/2001/src/elements.cc#newcode210
src/elements.cc:210: ASSERT(copy_size > 0 || copy_size ==
ElementsAccessor::kCopyToEnd ||
On 2012/03/12 15:33:28, Jakob wrote:
> I don't see how copy_size can ever be < 0 at this point. It can be == 0,
> however.

Done.

http://codereview.chromium.org/9663002/diff/2001/src/elements.cc#newcode212
src/elements.cc:212: ASSERT(copy_size + static_cast<int>(to_start) <=
to->length());
On 2012/03/12 15:33:28, Jakob wrote:
> Duplicate assertion (see line 207).

Done.

http://codereview.chromium.org/9663002/diff/2001/src/elements.cc#newcode221
src/elements.cc:221: to->set(i + to_start, hole, SKIP_WRITE_BARRIER);
On 2012/03/12 15:33:28, Jakob wrote:
> to->set_the_hole(i + to_start)?

Done.

http://codereview.chromium.org/9663002/diff/2001/src/elements.cc#newcode255
src/elements.cc:255: ASSERT(to->get(i) == hole);
On 2012/03/12 15:33:28, Jakob wrote:
> ASSERT(to->is_the_hole(i))?

Done.

http://codereview.chromium.org/9663002/diff/2001/src/elements.cc#newcode260
src/elements.cc:260: ASSERT(((copy_size + static_cast<int>(to_start)) <=
to->length() &&
On 2012/03/12 15:33:28, Jakob wrote:
> nit: you can remove at least one pair of parentheses.

Done.

http://codereview.chromium.org/9663002/diff/2001/src/elements.cc#newcode338
src/elements.cc:338: ASSERT(((copy_size + static_cast<int>(to_start)) <=
to->length() &&
On 2012/03/12 15:33:28, Jakob wrote:
> nit: you can remove at least one pair of parentheses.

Done.

http://codereview.chromium.org/9663002/diff/2001/src/objects.cc
File src/objects.cc (right):

http://codereview.chromium.org/9663002/diff/2001/src/objects.cc#newcode8543
src/objects.cc:8543: if (elements_kind != NON_STRICT_ARGUMENTS_ELEMENTS) {
I think it's theoretically possible, it least it may be in the future. The
motivation here is that this code should look just like the FixedArray case so
they can be merged together. There is no reason not to support this case, but
you're right, the later map and elements set need to be removed.
On 2012/03/12 15:33:28, Jakob wrote:
> I don't think this if/else block makes sense. Can
NON_STRICT_ARGUMENTS_ELEMENTS
> really be in DOUBLE mode? Also, there are unconditional calls
> "set_map(new_map);" and "set_elements(elems);" below, which contradict what we
> do up here.

Jakob Kummerow

LGTM with a question. http://codereview.chromium.org/9663002/diff/6001/src/builtins.cc File src/builtins.cc (right): http://codereview.chromium.org/9663002/diff/6001/src/builtins.cc#newcode647 src/builtins.cc:647: AssertNoAllocation no_gc; OK, then why ...

8 years, 9 months ago (2012-03-13 08:51:02 UTC) #5

danno

8 years, 9 months ago (2012-03-16 14:00:26 UTC) #6

Feedback addressed

http://codereview.chromium.org/9663002/diff/6001/src/builtins.cc
File src/builtins.cc (right):

http://codereview.chromium.org/9663002/diff/6001/src/builtins.cc#newcode647
src/builtins.cc:647: AssertNoAllocation no_gc;
On 2012/03/13 08:51:02, Jakob wrote:
> OK, then why is this AssertNoAllocation still there?

Done.

http://codereview.chromium.org/9663002/diff/6001/src/builtins.cc#newcode759
src/builtins.cc:759: AssertNoAllocation no_gc;
On 2012/03/13 08:51:02, Jakob wrote:
> And this?

Done.

http://codereview.chromium.org/9663002/diff/6001/src/builtins.cc#newcode833
src/builtins.cc:833: AssertNoAllocation no_gc;
On 2012/03/13 08:51:02, Jakob wrote:
> And this?

Done.

http://codereview.chromium.org/9663002/diff/6001/src/builtins.cc#newcode885
src/builtins.cc:885: AssertNoAllocation no_gc;
On 2012/03/13 08:51:02, Jakob wrote:
> And this?

Done.

Expand Messages | Collapse Messages