Short-circuit embedded cons strings.

Short-circuit embedded cons strings.

R=mstarzinger@chromium.org
BUG=

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

[Michael Starzinger, 2013-06-20 10:04:13]

https://codereview.chromium.org/17418003/diff/1/src/objects-visiting-inl.h
File src/objects-visiting-inl.h (right):

https://codereview.chromium.org/17418003/diff/1/src/objects-visiting-inl.h#ne...
src/objects-visiting-inl.h:237: if (object->IsConsString() &&
ConsString::cast(object)->IsFlat()) {
I don't think this is the correct place to do the short-circuit strings. It's
not feasible to add a special check for IsConstString() to every iterator that
could potentially encounter a cons string. This needs to be done by the actual
visitor function. I'll look into a cleaner solution for this.

[Yang, 2013-06-20 11:33:09]

On 2013/06/20 10:04:13, Michael Starzinger wrote:
> https://codereview.chromium.org/17418003/diff/1/src/objects-visiting-inl.h
> File src/objects-visiting-inl.h (right):
> 
>
https://codereview.chromium.org/17418003/diff/1/src/objects-visiting-inl.h#ne...
> src/objects-visiting-inl.h:237: if (object->IsConsString() &&
> ConsString::cast(object)->IsFlat()) {
> I don't think this is the correct place to do the short-circuit strings. It's
> not feasible to add a special check for IsConstString() to every iterator that
> could potentially encounter a cons string. This needs to be done by the actual
> visitor function. I'll look into a cleaner solution for this.

New version uploaded.

[Michael Starzinger, 2013-06-20 12:40:34]

https://codereview.chromium.org/17418003/diff/5001/src/objects-visiting-inl.h
File src/objects-visiting-inl.h (right):

https://codereview.chromium.org/17418003/diff/5001/src/objects-visiting-inl.h...
src/objects-visiting-inl.h:240: StaticVisitor::VisitPointer(heap,
rinfo->target_object_address());
Yes, this approach will work. But it requires a trailing update of the target
object, because on some architectures you are not going to get a direct pointer
into the code because the embedded object is stored split accross several
instructions. Something like the following is needed.

Look at PointersUpdatingVisitor::VisitEmbeddedPointer() for an example of how
this can be done.

[Yang, 2013-06-20 13:25:41]

On 2013/06/20 12:40:34, Michael Starzinger wrote:
> https://codereview.chromium.org/17418003/diff/5001/src/objects-visiting-inl.h
> File src/objects-visiting-inl.h (right):
> 
>
https://codereview.chromium.org/17418003/diff/5001/src/objects-visiting-inl.h...
> src/objects-visiting-inl.h:240: StaticVisitor::VisitPointer(heap,
> rinfo->target_object_address());
> Yes, this approach will work. But it requires a trailing update of the target
> object, because on some architectures you are not going to get a direct
pointer
> into the code because the embedded object is stored split accross several
> instructions. Something like the following is needed.
> 
> Look at PointersUpdatingVisitor::VisitEmbeddedPointer() for an example of how
> this can be done.

uploaded another patch set :)

[Michael Starzinger, 2013-06-20 23:25:27]

The comment in the objects-visiting-inl.h file should make it clear that this is
complexity that is barely executed (as our test suite apparently never triggers
two bugs in this change). This again begs the question whether all of this
complexity is actually necessary.

A third bug with this short-circuiting is that it could potentially introduce a
pointer from code-space to new-space, as one half of the cons-string could very
well be in new-space whereas the cons-string itself has been in old-space.

As discussed offline yesterday: I would very much prefer to just flatten the
string at compile-time since this only happens for constant folding of string
literals at compile-time. So unless you can show me cases where this complexity
is actually necessary, I'll stick to my position that we shouldn't short-circuit
embedded strings at all.

https://codereview.chromium.org/17418003/diff/9001/src/objects-visiting-inl.h
File src/objects-visiting-inl.h (right):

https://codereview.chromium.org/17418003/diff/9001/src/objects-visiting-inl.h...
src/objects-visiting-inl.h:240: Object** target_address =
rinfo->target_object_address();
This change is missing the slot recording for the reloc slot in the code.
Frankly it surprises me that this would pass the test suite.

Also I am not sure passing a non-heap slot to the StaticVisitor here will work,
because if the target happens to be in an evacuation candidate this stack(!)
slot will be recorded in the slots buffer.

[Yang, 2013-06-21 07:33:27]

I'm sold. I don't want to open the can of worms you just granted me a look into
:)

So here I updated the CL to create flat concat strings at graph building. We may
revisit this if it causes performance issues, but I expect none.

[Michael Starzinger, 2013-06-21 09:20:27]

LGTM.

[Yang, 2013-06-21 09:24:39]

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