Bug 160282 – StringView should have an explicit m_is8Bit field.

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Mark Lam]

The current implementation reserves 1 bit in the m_length field as a is16Bit flag.  As a result, a StringView is incapable of handling strings that have a length of a 32-bit in size.  This results in a mismatch with the expectations of String, StringImpl, and JavaScriptCore's JSString.

[Mark Lam]

<rdar://problem/27327943>

[Mark Lam]

Created attachment 284754 [details]
proposed patch.

[Mark Lam]

Created attachment 284756 [details]
proposed patch: added rdar links to ChangeLogs.

[Benjamin Poulain]

Comment on attachment 284756 [details]
proposed patch: added rdar links to ChangeLogs.

Thanks for fixing this.

[Geoffrey Garen]

Mark pointed out that, if we needed to save space, we could store the flag in the low bit of the characters pointer without losing information -- but StringView seems to be a temporary object whose size doesn't matter.

[Anders Carlsson]

(In reply to comment #5)
> Mark pointed out that, if we needed to save space, we could store the flag
> in the low bit of the characters pointer without losing information -- but
> StringView seems to be a temporary object whose size doesn't matter.

This won't work, the pointer is byte aligned. 

The reason we store it in the upper bit is to make it possible to put the length + pointer in two registers.

[Geoffrey Garen]

Anders explained on IRC that we originally used bit packing so we could pass StringView by value in two registers.

[Saam Barati]

(In reply to comment #7)
> Anders explained on IRC that we originally used bit packing so we could pass
> StringView by value in two registers.

Won't that still happen on 64-bit?

[Geoffrey Garen]

> > Anders explained on IRC that we originally used bit packing so we could pass
> > StringView by value in two registers.
> 
> Won't that still happen on 64-bit?

Yes.

[Anders Carlsson]

(In reply to comment #9)
> > > Anders explained on IRC that we originally used bit packing so we could pass
> > > StringView by value in two registers.
> > 
> > Won't that still happen on 64-bit?
> 
> Yes.

Did you verify this?

[Geoffrey Garen]

> Did you verify this?

http://agner.org/optimize/calling_conventions.pdf, page 20:

"Entire object is transferred in integer registers and/or XMM registers if the size is no bigger than 128 bits... Examples: int and float: RDI."

~/scratch> cat scratch.cc

struct Object
{
    unsigned x;
    bool y;
};

__attribute__((noinline)) void f(Object)
{
}

int main()
{
    Object o;
    f(o);
}

~/scratch> clang++ -o scratch scratch.cc && otool -tvV scratch
scratch:
(__TEXT,__text) section
__Z1f6Object:
0000000100000f60	pushq	%rbp
0000000100000f61	movq	%rsp, %rbp
0000000100000f64	movq	%rdi, -0x8(%rbp)
0000000100000f68	popq	%rbp
0000000100000f69	retq
0000000100000f6a	nopw	(%rax,%rax)
_main:
0000000100000f70	pushq	%rbp
0000000100000f71	movq	%rsp, %rbp
0000000100000f74	subq	$0x10, %rsp
0000000100000f78	movq	-0x8(%rbp), %rax
0000000100000f7c	movq	%rax, -0x10(%rbp)
0000000100000f80	movq	-0x10(%rbp), %rdi       ## Hi Anders!
0000000100000f84	callq	__Z1f6Object            ## f(Object)
0000000100000f89	xorl	%eax, %eax
0000000100000f8b	addq	$0x10, %rsp
0000000100000f8f	popq	%rbp
0000000100000f90	retq

[Mark Lam]

Created attachment 284806 [details]
x86_64 benchmark results.

[Mark Lam]

Created attachment 284807 [details]
x86 benchmark results.

[Mark Lam]

Benchmark results show that perf is neutral in aggregate with this patch.

[Geoffrey Garen]

Actually, StringView never passed in registers because it has non-trivial copy and move constructors -- and it still doesn't pass in registers :(.

[Mark Lam]

Landed in r203834: <http://trac.webkit.org/r203834>.

[Darin Adler]

(In reply to comment #15)
> Actually, StringView never passed in registers because it has non-trivial
> copy and move constructors -- and it still doesn't pass in registers :(.

I think part of that statement wrong.

I am almost sure that the original version did pass in registers.

The work done in the copy and move constructors is only needed for debugging, so we should fix this so that in production builds it passes in registers again. I believe I caused this problem when adding the underlying string debugging mechanism.

This should be easy to fix!

[Mark Lam]

(In reply to comment #17)
> (In reply to comment #15)
> > Actually, StringView never passed in registers because it has non-trivial
> > copy and move constructors -- and it still doesn't pass in registers :(.
> 
> I think part of that statement wrong.
> 
> I am almost sure that the original version did pass in registers.
> 
> The work done in the copy and move constructors is only needed for
> debugging, so we should fix this so that in production builds it passes in
> registers again. I believe I caused this problem when adding the underlying
> string debugging mechanism.
> 
> This should be easy to fix!

FYI, I'll address this in https://bugs.webkit.org/show_bug.cgi?id=160344.