mclow.lists added a comment.
Howard just pointed out to me that `__clear_and_shrink` should be noexcept -
otherwise we get the generation of an exception table and a call to `terminate`
in string's move assignment operator. Fixed in revision 333435.
Repository:
rCXX libc++
vsk added a comment.
Thanks @tvanslyke and @mclow.lists, landed as r327064.
Repository:
rCXX libc++
https://reviews.llvm.org/D41976
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
This revision was automatically updated to reflect the committed changes.
Closed by commit rCXX327064: Low-hanging fruit optimization in
string::__move_assign(). (authored by vedantk, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D41976?vs=130819=137647#toc
Repository:
mclow.lists added a comment.
In https://reviews.llvm.org/D41976#1031674, @vsk wrote:
> @mclow.lists is this still fine to commit? I can land it and watch the bots,
> if you'd like.
Sure. Please do so.
https://reviews.llvm.org/D41976
___
vsk added a comment.
Herald added a subscriber: christof.
@mclow.lists is this still fine to commit? I can land it and watch the bots, if
you'd like.
https://reviews.llvm.org/D41976
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
tvanslyke added a comment.
Hello. I would just like to follow up on the status of this patch. Is there
anything else that is needed from me? I just want to make sure that nobody is
waiting on me for anything; I'm still not 100% familiar with the process.
Thanks in advance.
tvanslyke updated this revision to Diff 130819.
tvanslyke added a comment.
Moved `__clear_and_shrink()` to live with the other public dunder methods and
adapted the associated test accordingly.
https://reviews.llvm.org/D41976
Files:
include/string
mclow.lists added a comment.
> Since __clear_and_shrink() is private
There's no reason it has to be private.
People aren't supposed to call anything with a reserved name.
https://reviews.llvm.org/D41976
___
cfe-commits mailing list
mclow.lists added a comment.
In https://reviews.llvm.org/D41976#982978, @tvanslyke wrote:
> I don't have commit access myself so I've added the test to the diff.
I'll commit it then.
https://reviews.llvm.org/D41976
___
cfe-commits mailing list
tvanslyke updated this revision to Diff 130775.
tvanslyke added a comment.
Since `__clear_and_shrink()` is private the test covers copy and move
assignment. I also ran the libcxx/strings/basic.string and std/strings tests
with a hard-coded `assert(__invariants());` at the end of
mclow.lists accepted this revision.
mclow.lists added a comment.
This revision is now accepted and ready to land.
This looks good to me.
Please add a test in test/libcxx/strings/string.modifiers and commit.
Something like this:
#include
#include
int main () {
std::string l =
tvanslyke updated this revision to Diff 129969.
tvanslyke added a comment.
Implemented changes to ensure string state is valid after calling
`__clear_and_shrink()`. Benchmark results are identical.
https://reviews.llvm.org/D41976
Files:
string
Index: string
tvanslyke added a comment.
Sorry if I'm spamming too much. Just to be clear, I agree completely with your
sentiment but I'm pretty sure that `__clear_and_shrink()` has exactly the same
effects as `clear(); shrink_to_fit();`. Maybe `reserve(0);` should should be
unconditionally calling
tvanslyke added a comment.
Just to elaborate, in `reserve(0)` after the deallocation there's a check `if
(__now_long)` which takes the else branch and only ends up calling
`__set_short_size(__sz); // __sz = 0 here`. So even without this diff the only
thing `reserve(0)` does after deallocating
tvanslyke added a comment.
I think the only thing we need is a call to __set_long_cap(0) in the body of
the if() to make the string be in a valid state, but if you follow the logic
for a call to reserve(0) (after having called clear()) it doesn't even end up
doing that (unless I'm missing
mclow.lists added a comment.
I'm a bit leery of this patch. Not because of what it's trying to do, but
rather, the introduction of a method `__clear_and_shrink` that leaves the
string in an invalid state. For all the uses that you put it to, I don't
think that's a problem (though I'm still
tvanslyke added a comment.
I can adapt it to the style guide if it is decided that it should be added.
https://reviews.llvm.org/D41976
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
lebedev.ri added a comment.
In https://reviews.llvm.org/D41976#975659, @tvanslyke wrote:
> Here are the results:
>
> Comparing old-copymove.json to new-copymove.json
> BenchmarkTime CPU Time Old
> Time New CPU Old CPU New
>
>
tvanslyke added a comment.
I'll add that the reason I felt compelled to submit this change was because
perf was saying most of my program, that only ever moved strings, was spending
a majority of it's time in string::reserve() calls. If for no other reason, I
think it's confusing for people
tvanslyke added a comment.
Here are the results:
Comparing old-copymove.json to new-copymove.json
BenchmarkTime CPU Time Old Time
New CPU Old CPU New
mclow.lists added a comment.
Can you share your benchmark results, please?
https://reviews.llvm.org/D41976
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
vsk added reviewers: EricWF, mclow.lists, hiraditya.
vsk added a comment.
Adding some folks who may be interested.
https://reviews.llvm.org/D41976
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
tvanslyke updated this revision to Diff 129685.
tvanslyke added a comment.
I went ahead and just pulled it out to a small inline member function and added
it to __copy_assign_alloc(). Removed some other redundancies.
https://reviews.llvm.org/D41976
Files:
string
Index: string
tvanslyke created this revision.
tvanslyke added a reviewer: howard.hinnant.
Herald added a subscriber: cfe-commits.
shrink_to_fit() ends up doing a lot work to get information that we already
know since we just called clear(). This change seems concise enough to be
worth the couple extra
24 matches
Mail list logo