Page MenuHomePhabricator

Partially inline basic_string::operator=(const basic_string&)
ClosedPublic

Authored by mvels on Feb 26 2020, 12:59 PM.

Details

Summary

This change partially inlines operator=(const basic_string&) where both the input and current instance are short strings, making the assignment a fixed length inlined memcpy.

Assignments where either of the strings are long are delegate to assign_no_alias<is_short>(), which is templated for the long / short branch already observed in the caller.

Stable:

--------------------------------------------------------------------------------
Benchmark                                     Time             CPU   Iterations
--------------------------------------------------------------------------------
BM_StringAssignStr_Empty_Opaque            2.65 ns         2.66 ns    263745536
BM_StringAssignStr_Empty_Transparent       2.95 ns         2.96 ns    236494848
BM_StringAssignStr_Small_Opaque            2.93 ns         2.94 ns    237301760
BM_StringAssignStr_Small_Transparent       2.69 ns         2.69 ns    265809920
BM_StringAssignStr_Large_Opaque            19.6 ns         19.6 ns     35573760
BM_StringAssignStr_Large_Transparent       19.1 ns         19.1 ns     36716544
BM_StringAssignStr_Huge_Opaque             1901 ns         1901 ns       364544
BM_StringAssignStr_Huge_Transparent        1889 ns         1889 ns       360448

Unstable

--------------------------------------------------------------------------------
Benchmark                                     Time             CPU   Iterations
--------------------------------------------------------------------------------
BM_StringAssignStr_Empty_Opaque            1.29 ns         1.29 ns    540454912
BM_StringAssignStr_Empty_Transparent       1.11 ns         1.12 ns    628482048
BM_StringAssignStr_Small_Opaque            1.29 ns         1.29 ns    541216768
BM_StringAssignStr_Small_Transparent       1.11 ns         1.11 ns    629469184
BM_StringAssignStr_Large_Opaque            15.6 ns         15.6 ns     44945408
BM_StringAssignStr_Large_Transparent       14.9 ns         14.9 ns     46764032
BM_StringAssignStr_Huge_Opaque             1713 ns         1713 ns       401408
BM_StringAssignStr_Huge_Transparent        1704 ns         1704 ns       397312

Diff Detail

Event Timeline

mvels created this revision.Feb 26 2020, 12:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 26 2020, 12:59 PM
mvels updated this revision to Diff 246823.Feb 26 2020, 1:03 PM

Fix left behind comment

EricWF requested changes to this revision.Feb 26 2020, 2:09 PM

@mvels Can you look over this diff?

There are two copies of each file in different locations.

Please update this review with a clean diff.

This revision now requires changes to proceed.Feb 26 2020, 2:09 PM
mvels updated this revision to Diff 246857.Feb 26 2020, 4:58 PM

Removed duplicated files

mvels added a comment.Feb 26 2020, 5:00 PM

@mvels Can you look over this diff?

There are two copies of each file in different locations.

Please update this review with a clean diff.

Picked a new diff, looks good afaics

ldionne requested changes to this revision.Feb 27 2020, 5:35 AM

Requesting changes so it shows up properly in my review queue (for question about assign). Otherwise this looks excellent.

libcxx/include/__string
168

Now I'm thinking that splitting stable an unstable was a *really* good decision. I don't even have to worry about the effects of this change on the stable ABI.

libcxx/include/string
2212

I love this

2292

Are there homologous changes we should make to the various assign methods?

This revision now requires changes to proceed.Feb 27 2020, 5:35 AM
EricWF accepted this revision.Feb 27 2020, 10:14 AM

LGTM after addressing @ldionne's comment about other assign methods that might benefit from this.

Though I'm happy to see those changes in another patch.

mvels marked 6 inline comments as done.Feb 27 2020, 5:50 PM
mvels added inline comments.
libcxx/include/__string
168

Yes, I agree. BTW: we still need to make the ifdef under an ABI flag as Eric pointed out, but those sre mostly cosmetics.

Eric and I were discussing ideas Eric had about a future where we are not having to deal with all this ABI pain, and where we don't need these absolute splits. I do think we can get there (eventually), but until then, this is my preferred least worse solution :)

libcxx/include/string
2212

Thanks, yes, it's an easy way to avoid branches, but also some other ideas that are in the pipeline that Eric and I discussed.

For example, a 'grow' function that is overloaded for short / long. Our current growth pattern is a bit awkward as going from an SSO to a long string, we have 23 * 2 = 46 as a min cap, leading to an awkward first step up of 48 for length in range [23-47] (which also awkwardly doubles to 96, etc). Going from SSO to either 32 or 64 is a cleaner and cheaper step up and cache line friendly.

2292

Yes, although I plan to have all these in separate incremental reviews. There may be some shuffling small adjustments of these functions going forward, but this being unstable, nothing we can not tinker with incrementally.

I'd also like to have my document shared soon (a mid week / weekend of skiing currently is in the way). For the 'regular' assign, there are some additional factors, for example, for assign(const value_type*), if we know the length at compile time, there is no need to exercise traits::length in the external code, we can special case those in a partial inlineable part (i.e., the submitted builtin_constant_p) which calls __assign_no_alias (as constant strings never alias). Ditto, if the string is at compile time known to be short, we can directly inline the assign, and have all other code elided, etc.

Like Eric imo these are best done incrementally

mvels marked 3 inline comments as done.Feb 27 2020, 5:51 PM
This revision was not accepted when it landed; it landed in state Needs Revision.Mar 2 2020, 7:04 AM
This revision was automatically updated to reflect the committed changes.