Make sure when we shrink an object that we store a filler first into the free memory before updatin…

Make sure when we shrink an object that we store a filler first into the free memory before updating the map or size.

BUG=
R=jarin@chromium.org

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

[Hannes Payer (out of office), 2014-03-31 09:42:27]

https://codereview.chromium.org/219103002/diff/1/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/219103002/diff/1/src/objects.h#newcode1814
src/objects.h:1814: // Get the map using acquire load.
We could templetize the getters and setters. I think it is better to have
explicit definitions. WDYT?

[Jarin, 2014-03-31 12:59:04]

As discussed in person, we still need to sync a bit more, see the comments
below. (Obviously, the sweeper will also need to synchronize, but I am expecting
that to come in a different CL.)

https://codereview.chromium.org/219103002/diff/40001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/219103002/diff/40001/src/objects-inl.h#newcod...
src/objects-inl.h:1432: // here.
Wrong comment? Perhaps just remove the comment.

https://codereview.chromium.org/219103002/diff/40001/src/objects-inl.h#newcod...
src/objects-inl.h:1446: // here.
Wrong comment? Remove...

https://codereview.chromium.org/219103002/diff/40001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/219103002/diff/40001/src/objects.cc#newcode2391
src/objects.cc:2391: object->no_barrier_set_map(*new_map);
Even if the size does not change, I think you should still synchronize so that
the sweeper sees the write of the new size (rather than some reordered previous
value).

https://codereview.chromium.org/219103002/diff/40001/src/objects.cc#newcode2476
src/objects.cc:2476: // change the actual object size.
This should state the same reason for no synchronization as the comment above
(line 2462). No change in size is not a reason for omitting sync -
compiler/hardware reordering could still screw us.

[Hannes Payer (out of office), 2014-03-31 13:23:15]

Yes, this is the first part. The concurrent GC counter-part is in the next CL.

https://codereview.chromium.org/219103002/diff/40001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/219103002/diff/40001/src/objects-inl.h#newcod...
src/objects-inl.h:1432: // here.
On 2014/03/31 12:59:05, jarin wrote:
> Wrong comment? Perhaps just remove the comment.

Done. And I also removed the original comments since it does not really provide
much information.

https://codereview.chromium.org/219103002/diff/40001/src/objects-inl.h#newcod...
src/objects-inl.h:1446: // here.
On 2014/03/31 12:59:05, jarin wrote:
> Wrong comment? Remove...

I folded the no_barrier methods into the general methods and dropped the
no_barrier* prefix.

https://codereview.chromium.org/219103002/diff/40001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/219103002/diff/40001/src/objects.cc#newcode2391
src/objects.cc:2391: object->no_barrier_set_map(*new_map);
On 2014/03/31 12:59:05, jarin wrote:
> Even if the size does not change, I think you should still synchronize so that
> the sweeper sees the write of the new size (rather than some reordered
previous
> value).

Good point. Done.

https://codereview.chromium.org/219103002/diff/40001/src/objects.cc#newcode2476
src/objects.cc:2476: // change the actual object size.
On 2014/03/31 12:59:05, jarin wrote:
> This should state the same reason for no synchronization as the comment above
> (line 2462). No change in size is not a reason for omitting sync -
> compiler/hardware reordering could still screw us.

Done.

[Jarin, 2014-03-31 13:30:00]

lgtm. Just one nit regarding comments.

https://codereview.chromium.org/219103002/diff/40001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/219103002/diff/40001/src/objects.cc#newcode2476
src/objects.cc:2476: // change the actual object size.
On 2014/03/31 13:23:15, Hannes Payer wrote:
> On 2014/03/31 12:59:05, jarin wrote:
> > This should state the same reason for no synchronization as the comment
above
> > (line 2462). No change in size is not a reason for omitting sync -
> > compiler/hardware reordering could still screw us.
> 
> Done.

I meant the comment to be changed. Of course, you can synchronize here, but then
you should update the comment above (and maybe the comment on line 2462) so that
it does not say that sync is not required.

[Hannes Payer (out of office), 2014-03-31 13:35:54]

https://codereview.chromium.org/219103002/diff/40001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/219103002/diff/40001/src/objects.cc#newcode2476
src/objects.cc:2476: // change the actual object size.
On 2014/03/31 13:30:01, jarin wrote:
> On 2014/03/31 13:23:15, Hannes Payer wrote:
> > On 2014/03/31 12:59:05, jarin wrote:
> > > This should state the same reason for no synchronization as the comment
> above
> > > (line 2462). No change in size is not a reason for omitting sync -
> > > compiler/hardware reordering could still screw us.
> > 
> > Done.
> 
> I meant the comment to be changed. Of course, you can synchronize here, but
then
> you should update the comment above (and maybe the comment on line 2462) so
that
> it does not say that sync is not required.

Absolutely. Changed it back and changed the comment. Done.

[Hannes Payer (out of office), 2014-03-31 14:29:11]

Committed patchset #5 manually as r20361 (presubmit successful).