Cleanup: Initialize GURL by moving std::string

Cleanup: Initialize GURL by moving std::string

Previously it used string::swap, but std::move is more effective in this case.

BUG=367418
TEST=
R=abarth@chromium.org,brettw@chromium.org

Committed: https://crrev.com/cebea5e61b469ae80032d9c45ffe1f086af6adc7
Cr-Commit-Position: refs/heads/master@{#397908}

[ki.stfu, 2015-09-21 05:55:55]

ki.stfu@gmail.com changed reviewers:
+ pkasting@chromium.org

[ki.stfu, 2015-09-21 05:55:55]

+pkasting

https://codereview.chromium.org/1360533002/diff/1/url/gurl.h
File url/gurl.h (right):

https://codereview.chromium.org/1360533002/diff/1/url/gurl.h#newcode51
url/gurl.h:51: // can convert from WebURL without copying the string. When we
call this
On 2015/09/20 21:58:20, Peter Kasting wrote:
> This sort of change is likely wrong.  The old code is very explicit about why
> it's passing by value, and changing that in the new code probably does the
> opposite of what you want, by forcing the compiler to generate an object whose
> address can be taken.
> 
> Unless whoever wrote the old comment was flat-out wrong, which I very much
> doubt, places like this should not be changed.
> 
> This was only the second file in this CL I looked at; it worries me that it
took
> 2 files to find a potential red flag.  Reviewers are likely to rubber-stamp
this
> sort of change, so you need to be very careful it's really right.
(the original message is here:
https://codereview.chromium.org/1349783006/diff/20001/url/gurl.h#newcode51)

Please take into account the 2nd version which uses movable arguments. In this
case, compiler moves an object and there is no copying of the string.
Initially I wanted to do only the version with movable std::string, but the 1st
is required for unittests.

Thus, it does not reduce the performance, but on the contrary, it causes the
compiler to move the object.

[brettw, 2015-09-21 06:01:24]

According to the style guide:
  https://chromium-cpp.appspot.com/
additions of rvalue references should be approved by the owners of the
styleguide directory. I think this is a good candidate for an exception (to
allow this change), but I want to be sure to follow the correct procedure and
also get input about this particular patch to make sure the rvalue ref usage is
correct (since I don't normally review such code)

Can you send a quick note to the people in styleguide/c++/OWNERS that we would
like to get an exception to the rvalue ref rule and point them to this patch?

[ki.stfu, 2015-10-01 07:20:52]

ki.stfu@gmail.com changed reviewers:
+ danakj@chromium.org, jamesr@chromium.org, thakis@chromium.org

[ki.stfu, 2015-10-01 07:20:52]

+danakj@chromium.org, jamesr@chromium.org, thakis@chromium.org

What about the use of rvalue reference in GURL::GURL?

Link to discussion:
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/BCONSC5ciBE

[Peter Kasting, 2016-04-29 02:08:39]

On 2015/10/01 07:20:52, ki.stfu wrote:
> mailto:+danakj@chromium.org, mailto:jamesr@chromium.org,
mailto:thakis@chromium.org
> 
> What about the use of rvalue reference in GURL::GURL?
> 
> Link to discussion:
>
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/BCONSC5ciBE

Styleguide owners, can you provide input here?  Would be nice to move this
moribund CL forward or else close.

[danakj, 2016-04-29 02:17:54]

https://codereview.chromium.org/1360533002/diff/1/url/gurl.cc
File url/gurl.cc (right):

https://codereview.chromium.org/1360533002/diff/1/url/gurl.cc#newcode113
url/gurl.cc:113: : spec_(std::move(canonical_spec)),
Doing this instead of swap is cool.

https://codereview.chromium.org/1360533002/diff/1/url/gurl.h
File url/gurl.h (right):

https://codereview.chromium.org/1360533002/diff/1/url/gurl.h#newcode58
url/gurl.h:58: GURL(std::string&& canonical_spec, const url::Parsed& parsed,
According to the google style guide the way to write this is "std::string", and
when you std::move() at the callsite, it will move the contents into this string
here.

Moving a string is pretty cheap, I'd expect that trying to avoid an extra move
here is not buying you much?

Do you have performance numbers justifying this?

[ki.stfu, 2016-05-25 16:53:33]

https://codereview.chromium.org/1360533002/diff/1/url/gurl.cc
File url/gurl.cc (right):

https://codereview.chromium.org/1360533002/diff/1/url/gurl.cc#newcode113
url/gurl.cc:113: : spec_(std::move(canonical_spec)),
On 2016/04/29 02:17:54, danakj wrote:
> Doing this instead of swap is cool.

I'm glad to make you happy! :)

https://codereview.chromium.org/1360533002/diff/1/url/gurl.h
File url/gurl.h (right):

https://codereview.chromium.org/1360533002/diff/1/url/gurl.h#newcode58
url/gurl.h:58: GURL(std::string&& canonical_spec, const url::Parsed& parsed,
On 2016/04/29 02:17:54, danakj wrote:
> According to the google style guide the way to write this is "std::string",
and
> when you std::move() at the callsite, it will move the contents into this
string
> here.
> 
I can't find a mention about this way in style guide.

> Moving a string is pretty cheap, I'd expect that trying to avoid an extra move
> here is not buying you much?
> 
Right.

> Do you have performance numbers justifying this?
No, I didn't measure it in practice. Just theoretical reasoning.

[danakj, 2016-05-25 19:59:37]

https://codereview.chromium.org/1360533002/diff/1/url/gurl.h
File url/gurl.h (right):

https://codereview.chromium.org/1360533002/diff/1/url/gurl.h#newcode58
url/gurl.h:58: GURL(std::string&& canonical_spec, const url::Parsed& parsed,
On 2016/05/25 16:53:33, ki.stfu wrote:
> On 2016/04/29 02:17:54, danakj wrote:
> > According to the google style guide the way to write this is "std::string",
> and
> > when you std::move() at the callsite, it will move the contents into this
> string
> > here.
> > 
> I can't find a mention about this way in style guide.

https://google.github.io/styleguide/cppguide.html#Rvalue_references

"Use rvalue references only to define move constructors and move assignment
operators, or for perfect forwarding."

i.e. Don't use it for passing to methods.

> > Moving a string is pretty cheap, I'd expect that trying to avoid an extra
move
> > here is not buying you much?
> > 
> Right.
> 
> > Do you have performance numbers justifying this?
> No, I didn't measure it in practice. Just theoretical reasoning.

[ki.stfu, 2016-05-26 05:15:30]

https://codereview.chromium.org/1360533002/diff/1/url/gurl.h
File url/gurl.h (right):

https://codereview.chromium.org/1360533002/diff/1/url/gurl.h#newcode58
url/gurl.h:58: GURL(std::string&& canonical_spec, const url::Parsed& parsed,
On 2016/05/25 19:59:37, danakj wrote:
> On 2016/05/25 16:53:33, ki.stfu wrote:
> > On 2016/04/29 02:17:54, danakj wrote:
> > > According to the google style guide the way to write this is
"std::string",
> > and
> > > when you std::move() at the callsite, it will move the contents into this
> > string
> > > here.
> > > 
> > I can't find a mention about this way in style guide.
> 
> https://google.github.io/styleguide/cppguide.html#Rvalue_references
> 
> "Use rvalue references only to define move constructors and move assignment
> operators, or for perfect forwarding."
> 
> i.e. Don't use it for passing to methods.
> 
> > > Moving a string is pretty cheap, I'd expect that trying to avoid an extra
> move
> > > here is not buying you much?
> > > 
> > Right.
> > 
> > > Do you have performance numbers justifying this?
> > No, I didn't measure it in practice. Just theoretical reasoning.
> 

Doesn't it look like a moving "for perfect forwarding"?

[brettw, 2016-05-26 23:18:04]

I think perfect forwarding is writing a function that exactly wraps some other
function which isn't this case.

I actually think GURL is a fundamental enough type (and also semantically is a
string in many ways, where you're "moving" the string into a URL object) that we
can make exceptions to the style guide for it if we can make common operations
faster or more clear.

I think that the rvalue refs in this case are actually much more readable than
declaring the current copy in the parameter. The pass-by-copy optimization is
unusual, obscure, and trips people up. I have gotten several patches from random
people over the years "fixing" this. The rvalue ref, even if it violates the
letter of the style guide, is very clear about what's happening.

I hated the original optimization because of it's obscurity, but WebURL -> GURL
conversion is an important enough case that I had to accept it. But I also think
that if rvalue refs existed at that time we probably would have used them
instead.

So for these reasons I think we should do this patch.

If we do this I think we can remove the long comment about WebURL from the
header. One advantage of the new system is that I think it's clear what's
happening without a comment.

[danakj, 2016-05-27 00:06:20]

On Wed, May 25, 2016 at 10:15 PM, ki.stfu@gmail.com via
codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote:

>
> https://codereview.chromium.org/1360533002/diff/1/url/gurl.h
> File url/gurl.h (right):
>
> https://codereview.chromium.org/1360533002/diff/1/url/gurl.h#newcode58
> url/gurl.h:58: GURL(std::string&& canonical_spec, const url::Parsed&
> parsed,
> On 2016/05/25 19:59:37, danakj wrote:
> > On 2016/05/25 16:53:33, ki.stfu wrote:
> > > On 2016/04/29 02:17:54, danakj wrote:
> > > > According to the google style guide the way to write this is
> "std::string",
> > > and
> > > > when you std::move() at the callsite, it will move the contents
> into this
> > > string
> > > > here.
> > > >
> > > I can't find a mention about this way in style guide.
> >
> > https://google.github.io/styleguide/cppguide.html#Rvalue_references
> >
> > "Use rvalue references only to define move constructors and move
> assignment
> > operators, or for perfect forwarding."
> >
> > i.e. Don't use it for passing to methods.
> >
> > > > Moving a string is pretty cheap, I'd expect that trying to avoid
> an extra
> > move
> > > > here is not buying you much?
> > > >
> > > Right.
> > >
> > > > Do you have performance numbers justifying this?
> > > No, I didn't measure it in practice. Just theoretical reasoning.
> >
>
> Doesn't it look like a moving "for perfect forwarding"?
>

Perfect forwarding is

template <typename T>
void Function(T&& t) { Other(std::forward<T>(t)); }

The T is unknown and may be a reference or not. The && there is not a
rvalue reference, it is a forwarding reference since T is templated, and
std::forward is used.

https://www.chromium.org/rvalue-references

If you are just taking of a known type and passing it on, the style guide
asks you to pass by value and std::move() that, or use a const-lvalue
reference, or a pointer.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2016-05-27 00:09:01]

On Thu, May 26, 2016 at 4:18 PM, <brettw@chromium.org> wrote:

> I think perfect forwarding is writing a function that exactly wraps some
> other
> function which isn't this case.
>
> I actually think GURL is a fundamental enough type (and also semantically
> is a
> string in many ways, where you're "moving" the string into a URL object)
> that we
> can make exceptions to the style guide for it if we can make common
> operations
> faster or more clear.
>

I'd like to see data showing this method is worth an exception. I'm all for
making chrome fast.

I think that the rvalue refs in this case are actually much more readable
> than
> declaring the current copy in the parameter. The pass-by-copy optimization
> is
> unusual, obscure, and trips people up. I have gotten several patches from
> random
> people over the years "fixing" this. The rvalue ref, even if it violates
> the
> letter of the style guide, is very clear about what's happening.
>

I think rvalue references are a good idea too, this is an argument made on
c-style, that rvalue references are safer/clearer, etc. So far the style
guide has not changed. I fully support you arguing with the google style
arbiters, cuz I'd like us to use rvalue refs more over time.


>
> I hated the original optimization because of it's obscurity, but WebURL ->
> GURL
> conversion is an important enough case that I had to accept it. But I also
> think
> that if rvalue refs existed at that time we probably would have used them
> instead.
>
> So for these reasons I think we should do this patch.
>

I don't think we have justification to violate the style guide without
performance data showing why.

If we do this I think we can remove the long comment about WebURL from the
> header. One advantage of the new system is that I think it's clear what's
> happening without a comment.
>
> https://codereview.chromium.org/1360533002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[brettw, 2016-05-28 01:05:33]

There was a thread on the internal C style list and I also did some research
(see https://www.youtube.com/watch?v=xnqTKD8uD64 starting at about the 1 hour to
1:30:00 mark).

It seems current standard style for constructors is to pass by value and use
std::move in the constructor rather than the swap. I believe this is exactly the
same performance-wise as the const&& version since there is no empty object
constructed and swapped from, and there is only one function required.

So:

1. Remove the long comment in gurl.h about WebString, replace it with a short
one that mentions this isn't a mistake and handles both move and copy.

2. Change the implementation of the existing function in gurl.cc to use move
like you did in your new && version.

3. Update the CL description.

4. Profit!

[ki.stfu, 2016-05-31 12:35:46]

Description was changed from

==========
Cleanup: Pass std::string as const reference from url/

Passing std::string by reference can prevent extra copying of object.

BUG=367418
TEST=
R=abarth@chromium.org,brettw@chromium.org
==========

to

==========
Cleanup: Initialize GURL by moving std::string

Previously it used string::swap, but std::move is more effective in this case.

BUG=367418
TEST=
R=abarth@chromium.org,brettw@chromium.org
==========

[ki.stfu, 2016-06-03 19:09:32]

Friendly ping

[Peter Kasting, 2016-06-03 19:47:44]

pkasting@chromium.org changed reviewers:
- pkasting@chromium.org

[Peter Kasting, 2016-06-03 19:47:45]

Removing myself as multiple other reviewers are more familiar with this stuff
than me.

[brettw, 2016-06-03 20:46:51]

lgtm

[ki.stfu, 2016-06-04 04:09:50]

The CQ bit was checked by ki.stfu@gmail.com

[commit-bot: I haz the power, 2016-06-04 04:09:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1360533002/20001

[commit-bot: I haz the power, 2016-06-04 07:05:49]

Description was changed from

==========
Cleanup: Initialize GURL by moving std::string

Previously it used string::swap, but std::move is more effective in this case.

BUG=367418
TEST=
R=abarth@chromium.org,brettw@chromium.org
==========

to

==========
Cleanup: Initialize GURL by moving std::string

Previously it used string::swap, but std::move is more effective in this case.

BUG=367418
TEST=
R=abarth@chromium.org,brettw@chromium.org
==========

[commit-bot: I haz the power, 2016-06-04 07:05:50]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-06-04 07:07:27]

Description was changed from

==========
Cleanup: Initialize GURL by moving std::string

Previously it used string::swap, but std::move is more effective in this case.

BUG=367418
TEST=
R=abarth@chromium.org,brettw@chromium.org
==========

to

==========
Cleanup: Initialize GURL by moving std::string

Previously it used string::swap, but std::move is more effective in this case.

BUG=367418
TEST=
R=abarth@chromium.org,brettw@chromium.org

Committed: https://crrev.com/cebea5e61b469ae80032d9c45ffe1f086af6adc7
Cr-Commit-Position: refs/heads/master@{#397908}
==========

[commit-bot: I haz the power, 2016-06-04 07:07:28]

Patchset 2 (id:??) landed as
https://crrev.com/cebea5e61b469ae80032d9c45ffe1f086af6adc7
Cr-Commit-Position: refs/heads/master@{#397908}