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.
<rdar://problem/27327943>
Created attachment 284754 [details] proposed patch.
Created attachment 284756 [details] proposed patch: added rdar links to ChangeLogs.
Comment on attachment 284756 [details] proposed patch: added rdar links to ChangeLogs. Thanks for fixing this.
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.
(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.
Anders explained on IRC that we originally used bit packing so we could pass StringView by value in two registers.
(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?
> > 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.
(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?
> 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
Created attachment 284806 [details] x86_64 benchmark results.
Created attachment 284807 [details] x86 benchmark results.
Benchmark results show that perf is neutral in aggregate with this patch.
Actually, StringView never passed in registers because it has non-trivial copy and move constructors -- and it still doesn't pass in registers :(.
Landed in r203834: <http://trac.webkit.org/r203834>.
(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!
(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.