Page MenuHomePhabricator

Optimize / partially inline basic_string copy constructor
ClosedPublic

Authored by mvels on Jan 3 2020, 8:01 AM.

Details

Summary

Splits copy constructor up inlining short initialization, outlining long initialization into __init_long() which is the externally instantiated slow path initialization.

Subsequently changing the copy ctor to be inlined (not externally instantiated) provides significant speed ups for short string initialization.

Generated code given:

void StringCopyCtor(void* mem, const std::string& s) {

std::string*p = new(mem) std::string{s};

}

asm:

cmp     byte ptr [rsi + 23], 0
js      .LBB0_2
mov     rax, qword ptr [rsi + 16]
mov     qword ptr [rdi + 16], rax
movups  xmm0, xmmword ptr [rsi]
movups  xmmword ptr [rdi], xmm0
ret

.LBB0_2:

jmp     std::basic_string::__init_long # TAILCALL

Benchmark:
BM_StringCopy_Empty 5.19ns ± 6% 1.50ns ± 8% -71.02% (p=0.000 n=10+10)
BM_StringCopy_Small 5.14ns ± 8% 1.53ns ± 7% -70.17% (p=0.000 n=10+10)
BM_StringCopy_Large 18.9ns ± 0% 19.3ns ± 0% +1.92% (p=0.000 n=10+10)
BM_StringCopy_Huge 309ns ± 1% 316ns ± 5% ~ (p=0.633 n=8+10)

Event Timeline

mvels created this revision.Jan 3 2020, 8:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 3 2020, 8:01 AM
mvels updated this revision to Diff 238841.Jan 17 2020, 11:48 AM
  • Optimize / partially inline basic_string assignment operator

Changed init_long to accept [s,n] following discussion with EricWF

There are a couple of places that now duplicate the body of __init_long. Would it make sense to deduplicate that code in some way?

Also, the plan is to make the __init_long function externally instantiated. But that will be done in a follow-up change since it involves the ABI.

Otherwise, this LGTM.

libcxx/include/string
799

This comment is a weird place. It's probably better suited on __init_long itself.

802

Numbers never age well in comments. I would put them in the commit message instead.

1550

The name of this function should express the intention that it's out-of-line and explicitly instantiated.

Historically not much attention was paid to how std::string handled inlining and explicit instantiation, and now that we're fixing that I want to make it a clear part of any change.

How about __init_long_out_of_line, __init_long_external, or __init_long_slow?

mvels updated this revision to Diff 238853.Jan 17 2020, 12:23 PM

Rename __init_long

mvels marked 4 inline comments as done.Jan 17 2020, 12:24 PM
mvels added inline comments.
libcxx/include/string
802

Removed reference, it will pop in the commit once we do the inlining :)

EricWF accepted this revision.Jan 17 2020, 1:47 PM

LGTM

This revision is now accepted and ready to land.Jan 17 2020, 1:47 PM
mgorny added a subscriber: mgorny.Jan 20 2020, 12:46 AM

I'm afraid this breaks stuff. TableGen built against libc++ after this change misbehaves:

[18/31] Building ARMGenSystemRegister.inc...
FAILED: lib/Target/ARM/ARMGenSystemRegister.inc 
cd /home/motus/netbsd8/netbsd8/build-stage2 && /home/motus/netbsd8/netbsd8/build-stage2/bin/llvm-tblgen -gen-searchable-tables -I /home/motus/netbsd8/netbsd8/llvm-project/llvm/lib/Target/ARM -I /home/motus/netbsd8/netbsd8/llvm-project/llvm/include -I /home/motus/netbsd8/netbsd8/llvm-project/llvm/lib/Target /home/motus/netbsd8/netbsd8/llvm-project/llvm/lib/Target/ARM/ARM.td --write-if-changed -o lib/Target/ARM/ARMGenSystemRegister.inc -d lib/Target/ARM/ARMGenSystemRegister.inc.d
/home/motus/netbsd8/netbsd8/llvm-project/llvm/lib/Target/ARM/ARM.td:1269:1: error: Reached EOF without matching #endif

^
#0 0x000000000049b032 llvm::sys::PrintStackTrace(llvm::raw_ostream&) (/home/motus/netbsd8/netbsd8/build-stage2/bin/llvm-tblgen+0x49b032)
#1 0x000000000049905c llvm::sys::RunSignalHandlers() (/home/motus/netbsd8/netbsd8/build-stage2/bin/llvm-tblgen+0x49905c)
#2 0x000000000049b5e0 SignalHandler(int) (/home/motus/netbsd8/netbsd8/build-stage2/bin/llvm-tblgen+0x49b5e0)
Stack dump:
0.      Program arguments: /home/motus/netbsd8/netbsd8/build-stage2/bin/llvm-tblgen -gen-searchable-tables -I /home/motus/netbsd8/netbsd8/llvm-project/llvm/lib/Target/ARM -I /home/motus/netbsd8/netbsd8/llvm-project/llvm/include -I /home/motus/netbsd8/netbsd8/llvm-project/llvm/lib/Target /home/motus/netbsd8/netbsd8/llvm-project/llvm/lib/Target/ARM/ARM.td --write-if-changed -o lib/Target/ARM/ARMGenSystemRegister.inc -d lib/Target/ARM/ARMGenSystemRegister.inc.d 
ninja: build stopped: subcommand failed.

http://lab.llvm.org:8014/builders/netbsd-amd64/builds/834/steps/ninja%20build%20local/logs/stdio