Update JSONReader to take base::StringPiece instead of std::string

Update JSONReader to take base::StringPiece instead of std::string

I also went through the source tree and fixed any now-unnecessary conversions
from base::StringPiece to std::string

BUG=114785
TEST=Manual


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=140177

[Aaron Boodman, 2012-04-15 21:04:29]

LGTM

wow, that was lucky that the JSONReader was already setup to use data().

https://chromiumcodereview.appspot.com/10003001/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/component_loader.h (right):

https://chromiumcodereview.appspot.com/10003001/diff/1/chrome/browser/extensi...
chrome/browser/extensions/component_loader.h:80: const;
no need to change this

[Robert Sesek, 2012-04-16 16:25:29]

Please do not land this. I'm making significant changes to the JSONReader, with
which this will interfere.

[Robert Sesek, 2012-04-17 14:40:31]

No, it doesn't. My change copies the entire input string exactly once and
then stores StringPiece inside base::Value, rather than copying smaller
strings to form the object tree. I benched it this morning, and it turns
out this approach is about 25% faster than just using StringPiece as the
input, which makes sense because fewer small allocations/copies are
happening in a loop. Are you concerned about something else besides wall
time with these large copies, or is it just about speed?

rsesek / @chromium.org

+chromium-reviews -- not sure why this wasn't there

On Mon, Apr 16, 2012 at 12:41 PM, Aaron Boodman <aa@chromium.org> wrote:

> All this change does is switch the input type from std::string to
> StringPiece, which seems like a good thing to do no matter what. In
> particular, we need it for extensions to reduce copies of gigantic
> strings. Does your change also make this change?
>
> - a
>
> On Mon, Apr 16, 2012 at 9:38 AM, Robert Sesek <rsesek@chromium.org> wrote:
> > No, mine is not yet out for review, though it will be this week. The
> change
> > will speed up the parser about 30-45% and has optimizations inside that
> use
> > StringPiece inside a custom base::Value subclass, rather than changing
> the
> > input string to be a StringPiece. That way small string allocations
> aren't
> > done in a loop, as done now when constructing the Value tree.
> >
> > I can bench my approach versus this, but considering my change is 1k+ LOC
> > and has been under development for two weeks, I'd rather not have to deal
> > with merge conflicts.
> >
> > rsesek / @chromium.org
> >
> >
> > On Mon, Apr 16, 2012 at 12:33 PM, Aaron Boodman <aa@chromium.org> wrote:
> >>
> >> This one?
> >> https://chromiumcodereview.appspot.com/9960077
> >>
> >> They are going to create merge conflicts with one another, but they
> >> don't logically conflict. Why can't whoever lands second do the simple
> >> merge?
> >>
> >> - a
> >>
> >> On Mon, Apr 16, 2012 at 9:25 AM,  <rsesek@chromium.org> wrote:
> >> > Please do not land this. I'm making significant changes to the
> >> > JSONReader,
> >> > with
> >> > which this will interfere.
> >> >
> >> > http://codereview.chromium.org/10003001/
> >
> >
>

[Aaron Boodman, 2012-04-18 22:53:57]

It's orthogonal:

You can see the associated bug for this same information, but
summarizing: There are many places in Chrome where we have large JSON
resources baked into Chrome. Since JSONReader requires a std::string
as an argument, we are forced to make a copy of the entire string just
to pass it into JSONReader. It would be better to pass it StringPiece.

- a

On Tue, Apr 17, 2012 at 7:40 AM, Robert Sesek <rsesek@chromium.org> wrote:
> No, it doesn't. My change copies the entire input string exactly once and
> then stores StringPiece inside base::Value, rather than copying smaller
> strings to form the object tree. I benched it this morning, and it turns out
> this approach is about 25% faster than just using StringPiece as the input,
> which makes sense because fewer small allocations/copies are happening in a
> loop. Are you concerned about something else besides wall time with these
> large copies, or is it just about speed?
>
> rsesek / @chromium.org
>
> +chromium-reviews -- not sure why this wasn't there
>
>
> On Mon, Apr 16, 2012 at 12:41 PM, Aaron Boodman <aa@chromium.org> wrote:
>>
>> All this change does is switch the input type from std::string to
>> StringPiece, which seems like a good thing to do no matter what. In
>> particular, we need it for extensions to reduce copies of gigantic
>> strings. Does your change also make this change?
>>
>> - a
>>
>> On Mon, Apr 16, 2012 at 9:38 AM, Robert Sesek <rsesek@chromium.org> wrote:
>> > No, mine is not yet out for review, though it will be this week. The
>> > change
>> > will speed up the parser about 30-45% and has optimizations inside that
>> > use
>> > StringPiece inside a custom base::Value subclass, rather than changing
>> > the
>> > input string to be a StringPiece. That way small string allocations
>> > aren't
>> > done in a loop, as done now when constructing the Value tree.
>> >
>> > I can bench my approach versus this, but considering my change is 1k+
>> > LOC
>> > and has been under development for two weeks, I'd rather not have to
>> > deal
>> > with merge conflicts.
>> >
>> > rsesek / @chromium.org
>> >
>> >
>> > On Mon, Apr 16, 2012 at 12:33 PM, Aaron Boodman <aa@chromium.org> wrote:
>> >>
>> >> This one?
>> >> https://chromiumcodereview.appspot.com/9960077
>> >>
>> >> They are going to create merge conflicts with one another, but they
>> >> don't logically conflict. Why can't whoever lands second do the simple
>> >> merge?
>> >>
>> >> - a
>> >>
>> >> On Mon, Apr 16, 2012 at 9:25 AM,  <rsesek@chromium.org> wrote:
>> >> > Please do not land this. I'm making significant changes to the
>> >> > JSONReader,
>> >> > with
>> >> > which this will interfere.
>> >> >
>> >> > http://codereview.chromium.org/10003001/
>> >
>> >
>
>

[Aaron Boodman, 2012-04-18 23:09:35]

On Wed, Apr 18, 2012 at 3:53 PM, Aaron Boodman <aa@chromium.org> wrote:
> It's orthogonal:
>
> You can see the associated bug for this same information, but
> summarizing: There are many places in Chrome where we have large JSON
> resources baked into Chrome. Since JSONReader requires a std::string
> as an argument, we are forced to make a copy of the entire string just
> to pass it into JSONReader. It would be better to pass it StringPiece.

I took a look at your patch - it looks like a cool optimization. But
when using it with resources, we end up copying the resource strings
once to create a string to pass into JSONParser, then JSONParser
immediately copies it again.

For input strings that are constant, it would be really neat for
JSONParser to be able to address directly into them, and not do any
copies at all.

- a

[Robert Sesek, 2012-04-20 17:24:41]

On 2012/04/18 23:09:35, Aaron Boodman wrote:
> On Wed, Apr 18, 2012 at 3:53 PM, Aaron Boodman <mailto:aa@chromium.org> wrote:
> > It's orthogonal:
> >
> > You can see the associated bug for this same information, but
> > summarizing: There are many places in Chrome where we have large JSON
> > resources baked into Chrome. Since JSONReader requires a std::string
> > as an argument, we are forced to make a copy of the entire string just
> > to pass it into JSONReader. It would be better to pass it StringPiece.
> 
> I took a look at your patch - it looks like a cool optimization. But
> when using it with resources, we end up copying the resource strings
> once to create a string to pass into JSONParser, then JSONParser
> immediately copies it again.
> 
> For input strings that are constant, it would be really neat for
> JSONParser to be able to address directly into them, and not do any
> copies at all.
> 
> - a

For constant strings, this change would definitely make a difference. In the
past few days I've changed the new parser's implementation a little so that now
it can support this change with little (or no) conflict.

I'm now okay with landing this. Thanks for your patience and bearing with me!

[commit-bot: I haz the power, 2012-05-01 03:04:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mitchellwrosen@chromium.org/10003001/9002

[commit-bot: I haz the power, 2012-05-01 03:04:31]

Presubmit check for 10003001-9002 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    net
    base

Presubmit checks took 2.4s to calculate.

[Aaron Boodman, 2012-05-01 03:19:19]

+willchan for base/OWNERS

[willchan no longer on Chromium, 2012-05-01 03:41:33]

lgtm

[commit-bot: I haz the power, 2012-05-01 03:45:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mitchellwrosen@chromium.org/10003001/9002

[commit-bot: I haz the power, 2012-05-01 08:32:24]

Try job failure for 10003001-9002 (retry) on mac_rel for steps "browser_tests,
net_unittests, unit_tests".
It's a second try, previously, steps "browser_tests, net_unittests, unit_tests"
failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[Aaron Boodman, 2012-05-07 21:20:02]

I think my LGTM from way back still stands, but there's another.

[commit-bot: I haz the power, 2012-05-09 03:14:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mitchellwrosen@chromium.org/10003001/3...

[commit-bot: I haz the power, 2012-05-09 06:13:54]

Try job failure for 10003001-30002 (retry) on win_rel for steps "browser_tests,
installer_util_unittests".
It's a second try, previously, steps "browser_tests, installer_util_unittests"
failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-06-02 02:26:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mitchellwrosen@chromium.org/10003001/3...

[commit-bot: I haz the power, 2012-06-02 04:40:41]

Change committed as 140177