[WIP] make GURL::path() return a StringPiece

[WIP] make GURL::path() return a StringPiece

Mostly a manual transform.

BUG=

[Charlie Harrison, 2016-11-07 20:19:36]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-07 20:20:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-07 20:37:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-07 20:37:53]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[Charlie Harrison, 2016-11-07 21:21:48]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-07 21:22:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-07 21:41:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-07 21:41:23]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Charlie Harrison, 2016-11-07 23:15:00]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-07 23:15:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-07 23:34:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-07 23:34:03]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[Charlie Harrison, 2016-11-07 23:54:20]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-07 23:54:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-08 00:11:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-08 00:11:46]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Charlie Harrison, 2016-11-08 01:02:09]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-08 01:02:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-08 01:49:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-08 01:49:26]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Charlie Harrison, 2016-11-08 01:51:27]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-08 01:51:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-08 02:28:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-08 02:28:52]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Charlie Harrison, 2016-11-08 02:32:13]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-08 02:32:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-08 04:09:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-08 04:09:01]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Charlie Harrison, 2016-11-08 22:42:43]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-08 22:43:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-09 02:10:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-09 02:10:22]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2016-11-09 02:31:42]

csharrison@chromium.org changed reviewers:
+ brettw@chromium.org

[Charlie Harrison, 2016-11-09 02:31:43]

Brett, what do you think about this? I feel like there are tons of unnecessary
copies of using path, host, etc. when we could be using host_piece, path_piece.
Can we make it fast by default?

I also feel like vending StringPiece will make people tend towards more
performant code in general, and conscious of overhead in surrounding code/apis.
For people that need strings, calling as_string() does not add too much line
noise (imo).

This is still a WIP because I have not properly #included StringPiece.

[vabr (Chromium), 2016-11-09 09:20:36]

vabr@chromium.org changed reviewers:
+ vabr@chromium.org

[vabr (Chromium), 2016-11-09 09:20:37]

FWIW, components/password_manager and chrome/browser/password_manager LGTM. I
appreciate making the StringPiece a default to nudge code authors into checking
whether a copy is needed.

Cheers,
Vaclav

[brettw, 2016-11-09 15:40:24]

I considered this at the time I wrote this.

I rejected this because most of these are not on anything close to the critical
path. Any critical path handling is likely to be in Blink which doesn't use
GURL. So this patch won't make Chrome faster in any measurable or material way.

But I was concerned that doing this does increase the liklihood of crashes in
material ways. String pieces are easy to screw up. I screwed up some callers
when I converted them to using the _piece() variants of the getters, and I think
you really need to be aware of the fact that you're using a string piece which
is why I think people should be forced to be explicit when they want them.

Given the ubiquity of the GURL class being used by such a large portion of the
team, the risks of this type of change seemed not worth the benefit to me.

[Charlie Harrison, 2016-11-09 15:56:57]

On 2016/11/09 15:40:24, brettw (ping on IM after 24h) wrote:
> I considered this at the time I wrote this.
> 
> I rejected this because most of these are not on anything close to the
critical
> path. Any critical path handling is likely to be in Blink which doesn't use
> GURL. So this patch won't make Chrome faster in any measurable or material
way.
Blink embedders in the renderer process use GURL, but they are a small subset of
GURL users and can probably be audited manually.
Also, we are frequently starting to see cases where the critical path is the
browser process IO thread (crbug.com/655585).

> 
> But I was concerned that doing this does increase the liklihood of crashes in
> material ways. String pieces are easy to screw up. I screwed up some callers
> when I converted them to using the _piece() variants of the getters, and I
think
> you really need to be aware of the fact that you're using a string piece which
> is why I think people should be forced to be explicit when they want them.
This is spot on, and if you look back a few PS you can see errors I made which
only ASAN caught.

> 
> Given the ubiquity of the GURL class being used by such a large portion of the
> team, the risks of this type of change seemed not worth the benefit to me.
SGTM I am convinced the risk of crashes makes this not worth it. I would really
like to get us in a place where our core APIs are low overhead by default, but I
think it will require a bit more thinking than this.

Thanks for the review :)

[brettw, 2016-11-09 16:56:52]

BTW I'm enthusiastic about changing callers to using the _piece() variants when
it's obvious they can do so. I did a pass for some of them a while ago.

An intermediate step (which I'm not convinced is necessary or helpful but we can
consider) is renaming the regular accessors host_as_string() or something to
make more clear that you're making a copy and so people might look at
alternatives.

[Charlie Harrison, 2016-11-09 17:15:04]

On 2016/11/09 16:56:52, brettw (ping on IM after 24h) wrote:
> BTW I'm enthusiastic about changing callers to using the _piece() variants
when
> it's obvious they can do so. I did a pass for some of them a while ago.
Sounds good. The dependent PS does this for some operator== checks and I'll
probably look for more easy cases.

> 
> An intermediate step (which I'm not convinced is necessary or helpful but we
can
> consider) is renaming the regular accessors host_as_string() or something to
> make more clear that you're making a copy and so people might look at
> alternatives.
Yeah, I'm thinking about that too. It is very heavy handed, but may help bring
about more cultural acceptance of StringPiece-based APIs (where they are
appropriate). //base and //net are good about including these, but other parts
of the code usually opts for string APIs. I haven't made up my mind about this
though.

[brettw, 2016-11-09 18:36:50]

On 2016/11/09 17:15:04, Charlie Harrison wrote:
> On 2016/11/09 16:56:52, brettw (ping on IM after 24h) wrote:
> > BTW I'm enthusiastic about changing callers to using the _piece() variants
> when
> > it's obvious they can do so. I did a pass for some of them a while ago.
> Sounds good. The dependent PS does this for some operator== checks and I'll
> probably look for more easy cases.
> 
> > 
> > An intermediate step (which I'm not convinced is necessary or helpful but we
> can
> > consider) is renaming the regular accessors host_as_string() or something to
> > make more clear that you're making a copy and so people might look at
> > alternatives.
> Yeah, I'm thinking about that too. It is very heavy handed, but may help bring
> about more cultural acceptance of StringPiece-based APIs (where they are
> appropriate). //base and //net are good about including these, but other parts
> of the code usually opts for string APIs. I haven't made up my mind about this
> though.

Yeah, outside of net I don't think it matters that much.

[mmenke, 2016-11-09 18:38:36]

On 2016/11/09 18:36:50, brettw (ping on IM after 24h) wrote:
> On 2016/11/09 17:15:04, Charlie Harrison wrote:
> > On 2016/11/09 16:56:52, brettw (ping on IM after 24h) wrote:
> > > BTW I'm enthusiastic about changing callers to using the _piece() variants
> > when
> > > it's obvious they can do so. I did a pass for some of them a while ago.
> > Sounds good. The dependent PS does this for some operator== checks and I'll
> > probably look for more easy cases.
> > 
> > > 
> > > An intermediate step (which I'm not convinced is necessary or helpful but
we
> > can
> > > consider) is renaming the regular accessors host_as_string() or something
to
> > > make more clear that you're making a copy and so people might look at
> > > alternatives.
> > Yeah, I'm thinking about that too. It is very heavy handed, but may help
bring
> > about more cultural acceptance of StringPiece-based APIs (where they are
> > appropriate). //base and //net are good about including these, but other
parts
> > of the code usually opts for string APIs. I haven't made up my mind about
this
> > though.
> 
> Yeah, outside of net I don't think it matters that much.

I though this whole thing was inspired by some blink code doing a lot of random
weird stuff to GURLs, affecting perf, particularly when there are a lot of
URLRequests?

[Charlie Harrison, 2016-11-09 18:41:30]

On 2016/11/09 18:38:36, mmenke wrote:
> On 2016/11/09 18:36:50, brettw (ping on IM after 24h) wrote:
> > On 2016/11/09 17:15:04, Charlie Harrison wrote:
> > > On 2016/11/09 16:56:52, brettw (ping on IM after 24h) wrote:
> > > > BTW I'm enthusiastic about changing callers to using the _piece()
variants
> > > when
> > > > it's obvious they can do so. I did a pass for some of them a while ago.
> > > Sounds good. The dependent PS does this for some operator== checks and
I'll
> > > probably look for more easy cases.
> > > 
> > > > 
> > > > An intermediate step (which I'm not convinced is necessary or helpful
but
> we
> > > can
> > > > consider) is renaming the regular accessors host_as_string() or
something
> to
> > > > make more clear that you're making a copy and so people might look at
> > > > alternatives.
> > > Yeah, I'm thinking about that too. It is very heavy handed, but may help
> bring
> > > about more cultural acceptance of StringPiece-based APIs (where they are
> > > appropriate). //base and //net are good about including these, but other
> parts
> > > of the code usually opts for string APIs. I haven't made up my mind about
> this
> > > though.
> > 
> > Yeah, outside of net I don't think it matters that much.
> 
> I though this whole thing was inspired by some blink code doing a lot of
random
> weird stuff to GURLs, affecting perf, particularly when there are a lot of
> URLRequests?

Yes but I think that code (blink embedder) is much easier to audit manually, and
is shrinking all the time (blink onion soup effort).

Eventually we are hoping to live in a world without content/renderer or
chrome/renderer.

[Charlie Harrison, 2016-11-10 19:49:29]

Description was changed from

==========
[WIP] make GURL::path() return a StringPiece

Mostly a manual transform.

BUG=
==========

to

==========
[WIP] make GURL::path() return a StringPiece

Mostly a manual transform.

BUG=
==========