libaddressinput - reduce number of copies in storage class by 2

libaddressinput - reduce number of copies in storage class by 2

1. on the way in (Put)
2. on the way out (Get)

BUG=337679
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=249102

[Evan Stade, 2014-01-27 17:32:40]

+dbeam for libaddressinput
+thakis for base/values.*

[Nico, 2014-01-27 17:35:42]

s/me/brettw/ for base/values.*

[brettw, 2014-01-28 03:00:23]

https://chromiumcodereview.appspot.com/148463003/diff/1/base/values.h
File base/values.h (right):

https://chromiumcodereview.appspot.com/148463003/diff/1/base/values.h#newcode94
base/values.h:94: virtual bool GetAsStringValue(const StringValue** out_value)
const;
This new name seems good but it doesn't follow the pattern of the rest of the
functions. I think for consistency this should be just "GetAsString" (or feel
free to change the other ones!).

https://chromiumcodereview.appspot.com/148463003/diff/1/base/values.h#newcode171
base/values.h:171: scoped_ptr<std::string> value_;
I assume you did this to avoid the extra copy when constructing from a passed-in
string for your use-case. But this adds an extra malloc for every existing user
of this class, and generally malloc will be much more expensive than copying a
random string. Are we sure that this is the correct tradeoff?

[Evan Stade, 2014-01-28 18:37:36]

https://codereview.chromium.org/148463003/diff/1/base/values.h
File base/values.h (right):

https://codereview.chromium.org/148463003/diff/1/base/values.h#newcode171
base/values.h:171: scoped_ptr<std::string> value_;
On 2014/01/28 03:00:24, brettw wrote:
> I assume you did this to avoid the extra copy when constructing from a
passed-in
> string for your use-case.

yes

> But this adds an extra malloc for every existing user
> of this class, and generally malloc will be much more expensive than copying a
> random string. Are we sure that this is the correct tradeoff?

hmm. Do you think I should create a second StringValue class instead?
HeapStringValue or some such?

I've also noticed there's a JSONStringValue class and I'd like to be able to
access its payload without copying, just as I've done here. But GetAsStringValue
won't work for that, so I guess I need GetAsStringRef(std::string** out_value)
instead of GetAsStringValue/value().

[Evan Stade, 2014-01-28 18:43:35]

https://codereview.chromium.org/148463003/diff/1/base/values.h
File base/values.h (right):

https://codereview.chromium.org/148463003/diff/1/base/values.h#newcode171
base/values.h:171: scoped_ptr<std::string> value_;
On 2014/01/28 18:37:36, Evan Stade wrote:
> On 2014/01/28 03:00:24, brettw wrote:
> > I assume you did this to avoid the extra copy when constructing from a
> passed-in
> > string for your use-case.
> 
> yes
> 
> > But this adds an extra malloc for every existing user
> > of this class, and generally malloc will be much more expensive than copying
a
> > random string. Are we sure that this is the correct tradeoff?
> 
> hmm. Do you think I should create a second StringValue class instead?
> HeapStringValue or some such?
> 
> I've also noticed there's a JSONStringValue class and I'd like to be able to
> access its payload without copying, just as I've done here. But
GetAsStringValue
> won't work for that, so I guess I need GetAsStringRef(std::string** out_value)
> instead of GetAsStringValue/value().

Actually, it looks like StringPiece can't return a reference to itself as a
string. Let me rethink this.

[Evan Stade, 2014-01-28 18:48:45]

https://chromiumcodereview.appspot.com/148463003/diff/1/base/values.h
File base/values.h (right):

https://chromiumcodereview.appspot.com/148463003/diff/1/base/values.h#newcode171
base/values.h:171: scoped_ptr<std::string> value_;
On 2014/01/28 18:43:35, Evan Stade wrote:
> On 2014/01/28 18:37:36, Evan Stade wrote:
> > On 2014/01/28 03:00:24, brettw wrote:
> > > I assume you did this to avoid the extra copy when constructing from a
> > passed-in
> > > string for your use-case.
> > 
> > yes
> > 
> > > But this adds an extra malloc for every existing user
> > > of this class, and generally malloc will be much more expensive than
copying
> a
> > > random string. Are we sure that this is the correct tradeoff?
> > 
> > hmm. Do you think I should create a second StringValue class instead?
> > HeapStringValue or some such?
> > 
> > I've also noticed there's a JSONStringValue class and I'd like to be able to
> > access its payload without copying, just as I've done here. But
> GetAsStringValue
> > won't work for that, so I guess I need GetAsStringRef(std::string**
out_value)
> > instead of GetAsStringValue/value().
> 
> Actually, it looks like StringPiece can't return a reference to itself as a
> string. Let me rethink this.

Well, even in lieu of that other optimization, I still think the one  in this
patch is helfpul. So the question, again, is do you want me to make a
HeapStringValue instead?

[brettw, 2014-01-29 01:07:42]

> Well, even in lieu of that other optimization, I still think the one  in this
> patch is helfpul. So the question, again, is do you want me to make a
> HeapStringValue instead?

I definitely don't want two. But there is not enough context for why you want
this change. You filed a bug "reduce number of copies". Why do you want to
reduce the number of copies? Are you making 10 million and it's really slow? I
have a hard time imagining this is actually a problem without data.

[Evan Stade, 2014-01-29 01:17:10]

On 2014/01/29 01:07:42, brettw wrote:
> > Well, even in lieu of that other optimization, I still think the one  in
this
> > patch is helfpul. So the question, again, is do you want me to make a
> > HeapStringValue instead?
> 
> I definitely don't want two. But there is not enough context for why you want
> this change. You filed a bug "reduce number of copies". Why do you want to
> reduce the number of copies? Are you making 10 million and it's really slow? I
> have a hard time imagining this is actually a problem without data.

No, we're only cutting out a couple of copies with this patch, but the strings
are very large (can be on the order of ~1MB).

[brettw, 2014-01-29 02:26:58]

What about adding const and non-const std::string& value() accessors to the
StringValue class. Then you can do value().swap() the contents of your string
into it?

[brettw, 2014-01-29 02:30:14]

On 2014/01/29 01:07:42, brettw wrote:
> > Well, even in lieu of that other optimization, I still think the one  in
this
> > patch is helfpul. So the question, again, is do you want me to make a
> > HeapStringValue instead?
> 
> I definitely don't want two. But there is not enough context for why you want
> this change. You filed a bug "reduce number of copies". Why do you want to
> reduce the number of copies? Are you making 10 million and it's really slow? I
> have a hard time imagining this is actually a problem without data.

Maybe we should call the accessor str() or something. value.value() to get the
string seems weird.

[Evan Stade, 2014-01-29 02:35:05]

On 2014/01/29 02:26:58, brettw wrote:
> What about adding const and non-const std::string& value() accessors to the
> StringValue class. Then you can do value().swap() the contents of your string
> into it?

ok (I assume you mean const std::string& and std::string* as non-const ref is
against the style guide AFAIK)

[Evan Stade, 2014-01-30 01:17:08]

updated, ptal

[Dan Beam, 2014-01-30 03:57:58]

lgtm

https://codereview.chromium.org/148463003/diff/310001/base/values.h
File base/values.h (right):

https://codereview.chromium.org/148463003/diff/310001/base/values.h#newcode158
base/values.h:158: // Returns a reference to |value_|.
^ needs updating it seems

[Evan Stade, 2014-01-31 04:16:58]

ping brettw

[Evan Stade, 2014-02-03 23:28:06]

ping brettw

[brettw, 2014-02-04 04:00:22]

base LGTM with Dan's comment fixed. Sorry for the delay, was on vacation.

[Evan Stade, 2014-02-04 18:20:46]

thanks Brett. Welcome back.

[Evan Stade, 2014-02-04 18:20:56]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2014-02-04 18:25:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/148463003/350001

[commit-bot: I haz the power, 2014-02-04 19:36:45]

Retried try job too often on linux_rel for step(s) base_unittests,
browser_tests, interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2014-02-04 19:40:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/148463003/350001

[commit-bot: I haz the power, 2014-02-05 00:41:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/148463003/350001

[Paweł Hajdan Jr., 2014-02-05 18:51:14]

The CQ bit was unchecked by phajdan.jr@chromium.org

[Paweł Hajdan Jr., 2014-02-05 19:16:32]

The CQ bit was checked by phajdan.jr@chromium.org

[commit-bot: I haz the power, 2014-02-05 19:23:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/148463003/350001

[commit-bot: I haz the power, 2014-02-05 20:42:18]

Change committed as 249102