This is an archive of the discontinued LLVM Phabricator instance.

Partially inline basic_string copy constructor in UNSTABLE
ClosedPublic

Authored by mvels on Jan 22 2020, 1:07 PM.

Details

Summary

This change splits the copy constructor up inlining short initialization, and explicitly outlining long initialization into __init_copy_ctor_external() which is the externally instantiated slow path.

For unstable ABI, this has the following changes:

  • remove basic_string(const basic_string&)
  • remove basic_string(const basic_string&, const Allocator&)
  • add __init_copy_ctor_external(const value_type*, size_type)

Quick local benchmark for Copy:

Master

---------------------------------------------------------------
Benchmark                    Time             CPU   Iterations
---------------------------------------------------------------
BM_StringCopy_Empty       3.50 ns         3.51 ns    199326720
BM_StringCopy_Small       3.50 ns         3.51 ns    199510016
BM_StringCopy_Large       15.7 ns         15.7 ns     45230080
BM_StringCopy_Huge        1503 ns         1503 ns       464896

With this change

---------------------------------------------------------------
Benchmark                    Time             CPU   Iterations
---------------------------------------------------------------
BM_StringCopy_Empty       1.99 ns         2.00 ns    356471808
BM_StringCopy_Small       3.29 ns         3.30 ns    203425792
BM_StringCopy_Large       13.3 ns         13.3 ns     52948992
BM_StringCopy_Huge        1472 ns         1472 ns       475136

Diff Detail

Event Timeline

mvels created this revision.Jan 22 2020, 1:07 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 22 2020, 1:07 PM
mvels retitled this revision from Splits copy constructor up inlining short initialization, outlining long initialization into __init_copy_ctor_external() which is the externally instantiated slow path. to Partially inline basic_string copy constructor in UNSTABLE.Feb 20 2020, 9:01 PM
mvels edited the summary of this revision. (Show Details)
EricWF added inline comments.Feb 23 2020, 4:56 PM
libcxx/test/std/strings/basic.string/string.cons/copy_shrunk_long.pass.cpp
3

This file needs to go under test/libcxx/... instead of test/std/.

The former directory is for libc++ specific tests, the latter is for tests that
should pass with all standard library implementations.

This patch should contain the changes to the unstable extern template list.

Also, the added test should pass both before and after your change, correct?
That is: does this change introduce a behavioral change, or is it simply an optimization?

Otherwise, this patch LGTM.

libcxx/include/string
1553

This isn't always externally instantiated.
And it doesn't assert that __s is null terminated (nor can it)

What's this comment really trying to say?

1907

It's a bit unfortunate that this function essentially duplicates __init(char*, size_type).

Can you write a short comment as to why this provides benefit over simply calling __init?
The formulation is essentially the same.

mvels updated this revision to Diff 247998.Mar 3 2020, 12:57 PM
mvels marked an inline comment as done.
mvels edited the summary of this revision. (Show Details)

Update comments

mvels marked an inline comment as done.Mar 3 2020, 12:57 PM
mvels updated this revision to Diff 248000.Mar 3 2020, 12:59 PM

Updated comments, synced with master

mvels updated this revision to Diff 248012.Mar 3 2020, 1:34 PM

Diff vs origin

mvels updated this revision to Diff 248021.Mar 3 2020, 1:49 PM
  • Moved test
mvels marked an inline comment as done.Mar 3 2020, 1:49 PM
EricWF accepted this revision.Mar 3 2020, 2:02 PM

LGTM.

This revision is now accepted and ready to land.Mar 3 2020, 2:02 PM
Harbormaster completed remote builds in B47967: Diff 248021.
mvels updated this revision to Diff 248048.Mar 3 2020, 2:48 PM

Rebased

This revision was automatically updated to reflect the committed changes.
mvels added a comment.Mar 4 2020, 2:45 PM

Thanks, arc is not my friend :\ Fixed the diff using --base=git:origin

mvels added a comment.Mar 4 2020, 2:46 PM

BTW, you could argue 'arc' was smart enough to recognize this was a rolled forward revert, and only diff against the original submit, alas :)