Change preference APIs to take std::string instead of const char*.

Change preference APIs to take std::string instead of const char*.

The string can't be NULL, and most callers have std::strings already, so this
makes the intent clearer and avoid unnecessary switch between std::string and
const char*. Right now, the prefs are stored in char*, then converted to
strings, then passed again as char*, only to be converted again, which
creates the unnecessary temporary strings.

For example, PrefValueStore::OnPrefValueChanged passes |key|, which is a
string, as a const char* to PrefValueStore::NotifyPrefChanged, which then
passes it to PrefNotifierImpl::OnPreferenceChanged as a string again.

Our tests show that this change reduces on average by ~9% the number of total
allocations in the browser process.

Notes:
- The underlying pref map in the PrefRegistry is based on string keys and
thus any call to it, e.g. find(), needs to convert to string anyways at the
bottom of the implementation so might as well force the string to be passed
into the API originally.
- A strict char* API, i.e. all the way into the pref map with a custom
comparator/etc., would make memory harder to manage since we can't strictly
force the caller to pass only pointers to constants in .rodata and would
thus require explicit memory management.
- Ideally, we would need a hybrid string class that manages the memory on
demand. That way, it can point to hard coded strings with the memory
management disabled or allocate a buffer that it will manage, to get the
best of both worlds.

BUG=438215
Committed: https://crrev.com/7da6e9da88541615b6e709baa4ed6b05c40fcdcf
Cr-Commit-Position: refs/heads/master@{#306514}

[Georges Khalil, 2014-11-21 16:40:24]

Patchset #1 (id:1) has been deleted

[Georges Khalil, 2014-11-21 16:45:38]

Patchset #1 (id:20001) has been deleted

[Georges Khalil, 2014-12-01 16:12:20]

georgesak@chromium.org changed reviewers:
+ gab@chromium.org, thakis@chromium.org

[Georges Khalil, 2014-12-01 16:12:20]

thakis@chromium.org: Please review changes in base/values.cc.

gab@chromium.org: Please review changes in base/prefs.

[Nico, 2014-12-01 16:57:03]

Code looks fine, but the CL description needs work – why is it useful to reduce
the number of allocations? Does this improve any metric we care about in any
way? Our allocators are pretty fast.

(I think the new code is an improvement over the old one since it has nicer
interfaces, so I'd lg this with a description along the lines of "change pref
abis to take std::strings instead of const char*s. They can't be NULL, and most
callers have std::strings already, so this makes the intent clearer." But unless
you have data that says otherwise, just reducing allocations for its own sake
doesn't look like a good goal to me.)

https://codereview.chromium.org/753603002/diff/40001/base/values.cc
File base/values.cc (left):

https://codereview.chromium.org/753603002/diff/40001/base/values.cc#oldcode425
base/values.cc:425: dictionary_.insert(std::make_pair(key, in_value));
Isn't the lhs better here? It requires on-stack allocation of a pair (which,
since it's a string pair, needs to go to the heap), but it saves a second
traversal of the tree to find the right insertion position.

(I guess it doesn't matter too much either way, unless there's a benchmark
saying otherwise.)

[Nico, 2014-12-01 17:02:08]

On 2014/12/01 16:57:03, Nico wrote:
> Code looks fine, but the CL description needs work – why is it useful to
reduce
> the number of allocations? Does this improve any metric we care about in any
> way? Our allocators are pretty fast.
> 
> (I think the new code is an improvement over the old one since it has nicer
> interfaces, so I'd lg this with a description along the lines of "change pref
> abis to take std::strings instead of const char*s. They can't be NULL, and
most
> callers have std::strings already, so this makes the intent clearer." But
unless
> you have data that says otherwise, just reducing allocations for its own sake
> doesn't look like a good goal to me.)

Heh, just found the chromium-dev thread on this where Ryan says roughly the same
thing :-)

[gab, 2014-12-01 19:12:11]

Thanks for the analysis and CL, general concern and suggestion below.

https://codereview.chromium.org/753603002/diff/40001/base/prefs/pref_value_st...
File base/prefs/pref_value_store.cc (left):

https://codereview.chromium.org/753603002/diff/40001/base/prefs/pref_value_st...
base/prefs/pref_value_store.cc:252: NotifyPrefChanged(key.c_str(), type);
General concern:

pref names constants are const char[] (i.e. const char*) in .rodata to begin
with.

So in general it should be possible to use them as const char* EVERYWHERE (i.e.
the opposite of what you're doing).

Forcing std::string is problematic in that you at least have to build a
std::string in the first place which may even be overkill.

e.g., on this specific line you now build a std::string but NotifyPrefChanged
uses a const char* still (i.e. the exact problem you want to avoid of building
strings unnecessarily).

I agree that using a pointer when NULL is not a possibility is noisy (i.e.
DCHECK(pref_name)'s everywhere).

Perhaps const char[] is the right type everywhere (which prevents NULL) for
pref_names.

[Nico, 2014-12-01 19:17:57]

On Mon, Dec 1, 2014 at 11:12 AM, <gab@chromium.org> wrote:

> Thanks for the analysis and CL, general concern and suggestion below.
>
>
> https://codereview.chromium.org/753603002/diff/40001/base/
> prefs/pref_value_store.cc
> File base/prefs/pref_value_store.cc (left):
>
> https://codereview.chromium.org/753603002/diff/40001/base/
> prefs/pref_value_store.cc#oldcode252
> base/prefs/pref_value_store.cc:252: NotifyPrefChanged(key.c_str(),
> type);
> General concern:
>
> pref names constants are const char[] (i.e. const char*) in .rodata to
> begin with.
>
> So in general it should be possible to use them as const char*
> EVERYWHERE (i.e. the opposite of what you're doing).
>

If we never build pref names at runtime, this is a good point. I don't know
if we do.


> Forcing std::string is problematic in that you at least have to build a
> std::string in the first place which may even be overkill.
>

I wouldn't worry too much about allocations unless you have proof that a
certain allocation is expensive.


> e.g., on this specific line you now build a std::string but
> NotifyPrefChanged uses a const char* still (i.e. the exact problem you
> want to avoid of building strings unnecessarily).
>
> I agree that using a pointer when NULL is not a possibility is noisy
> (i.e. DCHECK(pref_name)'s everywhere).
>
> Perhaps const char[] is the right type everywhere (which prevents NULL)
> for pref_names.
>

const char[] in an argument is the same as const char*, so you can pass
NULL for that too. (const char[5] isn't, but then you need to know the size
of the array which you can't so, you could have a template<int N> foo(const
char[N]), but now you have one instantiation per argument length so that's
not good either).

Probably good enough to have a handful DCHECKs and not worry about NULLs
too much (like the old code)


>
> https://codereview.chromium.org/753603002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[gab, 2014-12-01 19:29:15]

On 2014/12/01 19:17:57, Nico wrote:
> On Mon, Dec 1, 2014 at 11:12 AM,
<https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=gab@chromium.org> wrote:
> 
> > Thanks for the analysis and CL, general concern and suggestion below.
> >
> >
> > https://codereview.chromium.org/753603002/diff/40001/base/
> > prefs/pref_value_store.cc
> > File base/prefs/pref_value_store.cc (left):
> >
> > https://codereview.chromium.org/753603002/diff/40001/base/
> > prefs/pref_value_store.cc#oldcode252
> > base/prefs/pref_value_store.cc:252: NotifyPrefChanged(key.c_str(),
> > type);
> > General concern:
> >
> > pref names constants are const char[] (i.e. const char*) in .rodata to
> > begin with.
> >
> > So in general it should be possible to use them as const char*
> > EVERYWHERE (i.e. the opposite of what you're doing).
> >
> 
> If we never build pref names at runtime, this is a good point. I don't know
> if we do.

We try very hard not to. PrefService ensures that only registered prefs are
queried for.

And we have a TODO [1] that all prefs be registered on startup (i.e. not at
runtime); and all early registrations use constants AFAIK.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/base/prefs/pref_se...

> 
> 
> > Forcing std::string is problematic in that you at least have to build a
> > std::string in the first place which may even be overkill.
> >
> 
> I wouldn't worry too much about allocations unless you have proof that a
> certain allocation is expensive.
> 
> 
> > e.g., on this specific line you now build a std::string but
> > NotifyPrefChanged uses a const char* still (i.e. the exact problem you
> > want to avoid of building strings unnecessarily).
> >
> > I agree that using a pointer when NULL is not a possibility is noisy
> > (i.e. DCHECK(pref_name)'s everywhere).
> >
> > Perhaps const char[] is the right type everywhere (which prevents NULL)
> > for pref_names.
> >
> 
> const char[] in an argument is the same as const char*, so you can pass
> NULL for that too. (const char[5] isn't, but then you need to know the size
> of the array which you can't so, you could have a template<int N> foo(const
> char[N]), but now you have one instantiation per argument length so that's
> not good either).
> 
> Probably good enough to have a handful DCHECKs and not worry about NULLs
> too much (like the old code)
> 
> 
> >
> > https://codereview.chromium.org/753603002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to
https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=chromium-reviews+unsubscri....

[Georges Khalil, 2014-12-01 19:39:13]

Patchset #2 (id:60001) has been deleted

[Georges Khalil, 2014-12-01 20:30:01]

Patchset #3 (id:100001) has been deleted

[Georges Khalil, 2014-12-01 20:34:12]

PTAL.

https://codereview.chromium.org/753603002/diff/40001/base/prefs/pref_value_st...
File base/prefs/pref_value_store.cc (left):

https://codereview.chromium.org/753603002/diff/40001/base/prefs/pref_value_st...
base/prefs/pref_value_store.cc:252: NotifyPrefChanged(key.c_str(), type);
On 2014/12/01 19:12:11, gab wrote:
> General concern:
> 
> pref names constants are const char[] (i.e. const char*) in .rodata to begin
> with.
> 
> So in general it should be possible to use them as const char* EVERYWHERE
(i.e.
> the opposite of what you're doing).
> 
> Forcing std::string is problematic in that you at least have to build a
> std::string in the first place which may even be overkill.
> 
> e.g., on this specific line you now build a std::string but NotifyPrefChanged
> uses a const char* still (i.e. the exact problem you want to avoid of building
> strings unnecessarily).
> 
> I agree that using a pointer when NULL is not a possibility is noisy (i.e.
> DCHECK(pref_name)'s everywhere).
> 
> Perhaps const char[] is the right type everywhere (which prevents NULL) for
> pref_names.

But the key is already a string so no allocation will be done. What we are
avoiding is having a string being passed as const char* only to then have a
string being allocated again, which is the case here. NotifyPrefChanged calls
OnPreferenceChanged, the latter taking a string.

It's true that it the preference starts off as const char*, but what we are
trying to avoid is this:

const char* --> std::string --> c_str() --> std::string

The first allocation of string is inevitable, because the preferences are being
kept in a map where the key is std::string. It's the second one (and even
potential more) that we are avoiding.

https://codereview.chromium.org/753603002/diff/40001/base/values.cc
File base/values.cc (left):

https://codereview.chromium.org/753603002/diff/40001/base/values.cc#oldcode425
base/values.cc:425: dictionary_.insert(std::make_pair(key, in_value));
On 2014/12/01 16:57:02, Nico wrote:
> Isn't the lhs better here? It requires on-stack allocation of a pair (which,
> since it's a string pair, needs to go to the heap), but it saves a second
> traversal of the tree to find the right insertion position.
> 
> (I guess it doesn't matter too much either way, unless there's a benchmark
> saying otherwise.)

I'm removing this code from the CL, because it doesn't fit with the description
anymore.

I'll have some bench-marking done and resubmit this as a new CL.

[Nico, 2014-12-01 20:38:11]

lgtm

nit: Please wrap your CL description to ~78 columns, and try to follow the "one
short line overview, blank line, more description (if necessary)" format.

[Georges Khalil, 2014-12-01 20:52:05]

The CQ bit was checked by georgesak@chromium.org

[commit-bot: I haz the power, 2014-12-01 20:53:38]

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

[gab, 2014-12-01 21:13:01]

See comments below.

Out of scope for this CL, but thinking of the broader problem: it still seems
easy for someone to misuse such APIs, pass a string's c_str() to have it
implicitly converted to a string again even if both caller/API use strings.

If analysis shows that this is a big problem perhaps we should have an
ExplicitString type or something which only allows char* construction from
string literals (which siggi was suggesting we could potentially select for
through some kind of template-magic).

Cheers,
Gab

https://codereview.chromium.org/753603002/diff/120001/base/prefs/pref_change_...
File base/prefs/pref_change_registrar.cc (right):

https://codereview.chromium.org/753603002/diff/120001/base/prefs/pref_change_...
base/prefs/pref_change_registrar.cc:53:
service_->RemovePrefObserver(it->first.c_str(), this);
To get the full benefit, callers of those APIs should also be updated, any plans
here? (it's relatively easy to search for ._str() in base/prefs and fix those
(which may already help a bunch), but doing it at scale is harder).

https://codereview.chromium.org/753603002/diff/120001/base/prefs/pref_member.cc
File base/prefs/pref_member.cc (right):

https://codereview.chromium.org/753603002/diff/120001/base/prefs/pref_member....
base/prefs/pref_member.cc:41: DCHECK(prefs_->FindPreference(pref_name_.c_str()))
Once again, another example, picked at random while looking around, of a place
where changing the API wasn't sufficient as the impl still uses c_str() to pass
things around which will force re-conversions with your new API, i.e.:

Previously: |pref_name_| was passed down this low-level API as a pointer,
potentially without being promoted to a string.
Now: You are guaranteed to force |pref_name_| into a c_str() every time this is
called.

Fixing at least base/prefs to use its own API properly seems worthwhile (then
fixing external callers is harder as mentioned above).

https://codereview.chromium.org/753603002/diff/120001/base/prefs/pref_value_s...
File base/prefs/pref_value_store.cc (right):

https://codereview.chromium.org/753603002/diff/120001/base/prefs/pref_value_s...
base/prefs/pref_value_store.cc:132:
pref_notifier_->OnPreferenceChanged(path.c_str());
OnPreferenceChanged() takes a const std::string&

https://code.google.com/p/chromium/codesearch#chromium/src/base/prefs/pref_no...

https://codereview.chromium.org/753603002/diff/120001/base/prefs/pref_value_s...
base/prefs/pref_value_store.cc:134: pref_changed_callback_.Run(path.c_str());
So does pref_changed_callback_

https://codereview.chromium.org/753603002/diff/120001/base/prefs/testing_pref...
File base/prefs/testing_pref_service.h (right):

https://codereview.chromium.org/753603002/diff/120001/base/prefs/testing_pref...
base/prefs/testing_pref_service.h:167: RemoveRecommendedPref(const std::string&
path) {
I assume this wrapping is git cl format's style choice?

[gab, 2014-12-01 21:13:20]

The CQ bit was unchecked by gab@chromium.org

[gab, 2014-12-01 21:14:38]

Oh also, as a general Chrome flow tip, do not put multiple reviewers if you
don't explicitly intend to wait for a review from them (i.e. don't CQ with a
pending reviewer).

No harm done, just a tip going forward.

[Nico, 2014-12-01 21:18:18]

On Mon, Dec 1, 2014 at 1:14 PM, <gab@chromium.org> wrote:

> Oh also, as a general Chrome flow tip, do not put multiple reviewers if you
> don't explicitly intend to wait for a review from them (i.e. don't CQ with
> a
> pending reviewer).
>

(To add some color: I usually wait for first lg and then cq. If I have
multiple people leaving comments – like here – I do wait for lgs from
everyone though. Rietveld now supports prefixing reviewers with * to mark
"you are considered mandatory reviewer", so the absence kind of suggests
"first to review wins". I do agree that one should usually wait – some time
proportional to the overall length of the review – when there are multiple
active reviewers.)


>
> No harm done, just a tip going forward.
>
> https://codereview.chromium.org/753603002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Georges Khalil, 2014-12-01 21:27:57]

On 2014/12/01 21:18:18, Nico wrote:
> On Mon, Dec 1, 2014 at 1:14 PM, <mailto:gab@chromium.org> wrote:
> 
> > Oh also, as a general Chrome flow tip, do not put multiple reviewers if you
> > don't explicitly intend to wait for a review from them (i.e. don't CQ with
> > a
> > pending reviewer).
> >
> 
> (To add some color: I usually wait for first lg and then cq. If I have
> multiple people leaving comments – like here – I do wait for lgs from
> everyone though. Rietveld now supports prefixing reviewers with * to mark
> "you are considered mandatory reviewer", so the absence kind of suggests
> "first to review wins". I do agree that one should usually wait – some time
> proportional to the overall length of the review – when there are multiple
> active reviewers.)
> 
> 
> >
> > No harm done, just a tip going forward.
> >
> > https://codereview.chromium.org/753603002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

My apologies, lesson learned :)

Gab, for your comments about more changes, I can certainly do that. I'll do it
in another CL and reference this one.

Does that work for you?

[Georges Khalil, 2014-12-01 21:28:21]

https://codereview.chromium.org/753603002/diff/120001/base/prefs/testing_pref...
File base/prefs/testing_pref_service.h (right):

https://codereview.chromium.org/753603002/diff/120001/base/prefs/testing_pref...
base/prefs/testing_pref_service.h:167: RemoveRecommendedPref(const std::string&
path) {
On 2014/12/01 21:13:01, gab wrote:
> I assume this wrapping is git cl format's style choice?

Correct.

[gab, 2014-12-01 21:56:17]

Yes additional changes in a follow-up CL sgtm, some comments were about new
c_str() calls added that were incorrect though, so remove those in this CL plz
:)

[Finnur, 2014-12-02 11:13:55]

Neat. :)

Couple of drive-by nits on the CL description:

> Change preference abis

s/abis/APIs/ ?

Also, is there a BUG reference you can link to?

[gab, 2014-12-02 13:41:56]

CC Bernhard FYI.

@Georges: can you please re-add the motivation behind this change (~12% strings
less allocs IIRC by avoiding conversion back-and-forth between char* and string)
to the CL description.

Cheers,
Gab

[Bernhard Bauer, 2014-12-02 13:52:54]

+1 to what Gab said. Having a type that implies string literals would be
awesome. AFAIK, this was the original motivation for using const char[] in the
API. And yes, if we use std::string in the API, we should use it everywhere and
get rid of those .c_str() calls.

[Georges Khalil, 2014-12-02 17:57:00]

PTAL.

I went through all the prefs code and eliminated all calls to c_str(). String
are now passed as const string& all the way.

I also updated the description, as per GAB's request.

https://codereview.chromium.org/753603002/diff/120001/base/prefs/pref_change_...
File base/prefs/pref_change_registrar.cc (right):

https://codereview.chromium.org/753603002/diff/120001/base/prefs/pref_change_...
base/prefs/pref_change_registrar.cc:53:
service_->RemovePrefObserver(it->first.c_str(), this);
On 2014/12/01 21:13:01, gab wrote:
> To get the full benefit, callers of those APIs should also be updated, any
plans
> here? (it's relatively easy to search for ._str() in base/prefs and fix those
> (which may already help a bunch), but doing it at scale is harder).

Done.

https://codereview.chromium.org/753603002/diff/120001/base/prefs/pref_member.cc
File base/prefs/pref_member.cc (right):

https://codereview.chromium.org/753603002/diff/120001/base/prefs/pref_member....
base/prefs/pref_member.cc:41: DCHECK(prefs_->FindPreference(pref_name_.c_str()))
On 2014/12/01 21:13:01, gab wrote:
> Once again, another example, picked at random while looking around, of a place
> where changing the API wasn't sufficient as the impl still uses c_str() to
pass
> things around which will force re-conversions with your new API, i.e.:
> 
> Previously: |pref_name_| was passed down this low-level API as a pointer,
> potentially without being promoted to a string.
> Now: You are guaranteed to force |pref_name_| into a c_str() every time this
is
> called.
> 
> Fixing at least base/prefs to use its own API properly seems worthwhile (then
> fixing external callers is harder as mentioned above).

Done.

https://codereview.chromium.org/753603002/diff/120001/base/prefs/pref_value_s...
File base/prefs/pref_value_store.cc (right):

https://codereview.chromium.org/753603002/diff/120001/base/prefs/pref_value_s...
base/prefs/pref_value_store.cc:132:
pref_notifier_->OnPreferenceChanged(path.c_str());
On 2014/12/01 21:13:01, gab wrote:
> OnPreferenceChanged() takes a const std::string&
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/prefs/pref_no...

Done.

https://codereview.chromium.org/753603002/diff/120001/base/prefs/pref_value_s...
base/prefs/pref_value_store.cc:134: pref_changed_callback_.Run(path.c_str());
On 2014/12/01 21:13:01, gab wrote:
> So does pref_changed_callback_

Done.

[gab, 2014-12-02 18:56:54]

lgtm, please also add these items to the CL description:

1) One of the example problematic call sequences you showed me in-person
yesterday where one call would alternate 4 times between char* and string.

2) An explanation as to why we can't just make the entire API use char*, i.e. as
discussed yesterday: 
 - The underlying pref map in the PrefRegistry is based on string keys and thus
any call to it, e.g. find(), needs to convert to string anyways at the bottom of
the impl so might as well force the string to be passed into the API originally.
 - A strict char* API, i.e. all the way into the pref map with a custom
comparator/etc., would make memory harder to manage since we can't strictly
force the caller to pass only pointers to constants in .rodata and would thus
require explicit memory management.


And stressing once again that ideally we'd have a type that knows how to handle
string literals without copying the buffer (maybe std::string is already smart
enough for this IDK), avoiding even more copies, and also enforcing that nobody
who owns a string mistakenly hands us their c_str() which the API would
immediately convert back (as template-magic would only allow char*
initialization from string literals).
Benchmarks may be required to justify such a type though...

Cheers!
Gab

PS: I like the investigation and work you guys are doing with Syzygy tools very
much :-)! Onwards to a better Chrome!

[Georges Khalil, 2014-12-02 22:44:15]

On 2014/12/02 18:56:54, gab wrote:
> lgtm, please also add these items to the CL description:
> 
> 1) One of the example problematic call sequences you showed me in-person
> yesterday where one call would alternate 4 times between char* and string.
> 
> 2) An explanation as to why we can't just make the entire API use char*, i.e.
as
> discussed yesterday: 
>  - The underlying pref map in the PrefRegistry is based on string keys and
thus
> any call to it, e.g. find(), needs to convert to string anyways at the bottom
of
> the impl so might as well force the string to be passed into the API
originally.
>  - A strict char* API, i.e. all the way into the pref map with a custom
> comparator/etc., would make memory harder to manage since we can't strictly
> force the caller to pass only pointers to constants in .rodata and would thus
> require explicit memory management.
> 
> 
> And stressing once again that ideally we'd have a type that knows how to
handle
> string literals without copying the buffer (maybe std::string is already smart
> enough for this IDK), avoiding even more copies, and also enforcing that
nobody
> who owns a string mistakenly hands us their c_str() which the API would
> immediately convert back (as template-magic would only allow char*
> initialization from string literals).
> Benchmarks may be required to justify such a type though...
> 
> Cheers!
> Gab
> 
> PS: I like the investigation and work you guys are doing with Syzygy tools
very
> much :-)! Onwards to a better Chrome!

CL description updated, committing.

And thanks, we are trying to make Chrome leaner! :)

[Georges Khalil, 2014-12-02 22:48:14]

The CQ bit was checked by georgesak@chromium.org

[commit-bot: I haz the power, 2014-12-02 22:50:03]

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

[commit-bot: I haz the power, 2014-12-03 01:10:36]

Committed patchset #4 (id:140001)

[commit-bot: I haz the power, 2014-12-03 01:11:18]

Patchset 4 (id:??) landed as
https://crrev.com/7da6e9da88541615b6e709baa4ed6b05c40fcdcf
Cr-Commit-Position: refs/heads/master@{#306514}