Another performance improvement for phone library - at least +25% to previous cl (982ms for 100 i...

Another performance improvement for phone library - at least +25% to previous cl (982ms for 100 items DB test comared to 1251ms)
But feels like much more as the penalty shifted to DB thread.
TEST=The caching object is tested by multiple AutofillManager, PersonalDataManager, etc. unittests + new unit-test to test performance.
BUG=85152
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=90434

[GeorgeY, 2011-06-10 05:23:15]

Separated parsed phone caching into separate cl.

[Ilya Sherman, 2011-06-10 07:17:33]

http://codereview.chromium.org/7044102/diff/4001/chrome/browser/autofill/phon...
File chrome/browser/autofill/phone_number_i18n.cc (right):

http://codereview.chromium.org/7044102/diff/4001/chrome/browser/autofill/phon...
chrome/browser/autofill/phone_number_i18n.cc:291: }
nit: The else stmt here makes the code a little bit more confusing to read.  How
about moving this logic into GetWholeNumber()?

http://codereview.chromium.org/7044102/diff/4001/chrome/browser/autofill/phon...
chrome/browser/autofill/phone_number_i18n.cc:383: PhoneObject&
PhoneObject::operator=(const PhoneObject& other) {
This is not currently a no-op when this == &other, but it should be.

http://codereview.chromium.org/7044102/diff/4001/chrome/browser/autofill/phon...
chrome/browser/autofill/phone_number_i18n.cc:396: delete i18n_number_;
How about making i18n_number_ be a scoped_ptr<> instead, and using the reset()
method?

http://codereview.chromium.org/7044102/diff/4001/chrome/browser/autofill/phon...
File chrome/browser/autofill/phone_number_i18n.h (right):

http://codereview.chromium.org/7044102/diff/4001/chrome/browser/autofill/phon...
chrome/browser/autofill/phone_number_i18n.h:118: private:
nit: Please leave a blank line above "private:"

[dhollowa, 2011-06-10 15:13:27]

http://codereview.chromium.org/7044102/diff/4001/chrome/browser/autofill/phon...
File chrome/browser/autofill/phone_number_i18n.h (right):

http://codereview.chromium.org/7044102/diff/4001/chrome/browser/autofill/phon...
chrome/browser/autofill/phone_number_i18n.h:17: namespace phonenumbers {
nit: namespaces don't indent when nested.

[GeorgeY, 2011-06-10 22:36:35]

http://codereview.chromium.org/7044102/diff/4001/chrome/browser/autofill/phon...
File chrome/browser/autofill/phone_number_i18n.cc (right):

http://codereview.chromium.org/7044102/diff/4001/chrome/browser/autofill/phon...
chrome/browser/autofill/phone_number_i18n.cc:291: }
On 2011/06/10 07:17:33, Ilya Sherman wrote:
> nit: The else stmt here makes the code a little bit more confusing to read. 
How
> about moving this logic into GetWholeNumber()?

Commented.
To move logic into GetWholeNumber() I would need additional data member which
will not be used for anything else, which is wasteful.

http://codereview.chromium.org/7044102/diff/4001/chrome/browser/autofill/phon...
chrome/browser/autofill/phone_number_i18n.cc:383: PhoneObject&
PhoneObject::operator=(const PhoneObject& other) {
On 2011/06/10 07:17:33, Ilya Sherman wrote:
> This is not currently a no-op when this == &other, but it should be.

Sure, but most compilers are smart enough to auto optimize copy constructors.

http://codereview.chromium.org/7044102/diff/4001/chrome/browser/autofill/phon...
chrome/browser/autofill/phone_number_i18n.cc:396: delete i18n_number_;
On 2011/06/10 07:17:33, Ilya Sherman wrote:
> How about making i18n_number_ be a scoped_ptr<> instead, and using the reset()
> method?

As I explained in the first review, there are some problems with exposing the
type in the header (type has to be complete for scoped_ptr<>), for example, one
of the problems is that phonenumber.pb.h is in root protobuf output folder (as
required by this version of the libphonenumber) so including it directly would
conflict with our coding style.

http://codereview.chromium.org/7044102/diff/4001/chrome/browser/autofill/phon...
File chrome/browser/autofill/phone_number_i18n.h (right):

http://codereview.chromium.org/7044102/diff/4001/chrome/browser/autofill/phon...
chrome/browser/autofill/phone_number_i18n.h:17: namespace phonenumbers {
On 2011/06/10 15:13:27, dhollowa wrote:
> nit: namespaces don't indent when nested. 

Done.

http://codereview.chromium.org/7044102/diff/4001/chrome/browser/autofill/phon...
chrome/browser/autofill/phone_number_i18n.h:118: private:
On 2011/06/10 07:17:33, Ilya Sherman wrote:
> nit: Please leave a blank line above "private:"

Done.

[GeorgeY, 2011-06-10 23:24:50]

Fixed scoped_ptr<> as well - it asked me for completed type, because it was
autogenerating destructor.

[Ilya Sherman, 2011-06-11 03:46:21]

Profiling results:  With the ComparePhones() cache, the cost is something like
1080ms on the DB thread and 10ms on the UI thread.  WIthout it, the cost is
something like 1370ms on the DB thread and 50ms on the UI thread.

[GeorgeY, 2011-06-18 00:35:41]

On 2011/06/11 03:46:21, Ilya Sherman wrote:
> Profiling results:  With the ComparePhones() cache, the cost is something like
> 1080ms on the DB thread and 10ms on the UI thread.  WIthout it, the cost is
> something like 1370ms on the DB thread and 50ms on the UI thread.

Please review again

[Ilya Sherman, 2011-06-18 00:47:14]

I think the profiling suggests that the PhoneObject cache is not necessarily
worth the additional code complexity; but if we want to keep it:

http://codereview.chromium.org/7044102/diff/8001/chrome/browser/autofill/phon...
File chrome/browser/autofill/phone_number_i18n.cc (right):

http://codereview.chromium.org/7044102/diff/8001/chrome/browser/autofill/phon...
chrome/browser/autofill/phone_number_i18n.cc:131: void CachePhone(const
string16& key, const autofill_i18n::PhoneObject& n);
Why not combine these two methods together, so that the logic of "cache if
unavailable" is encapsulated in the cache object rather than exposed to the
client?

[GeorgeY, 2011-06-20 22:59:11]

TEST: AutofillManagerTest.DeterminePossibleFieldTypesForUpload
Average of five runs, rounded to the nearest ms:
                 with cache without cache difference
50 profiles      589        911           +55%
500 profiles     4116       7068          +72%

http://codereview.chromium.org/7044102/diff/8001/chrome/browser/autofill/phon...
File chrome/browser/autofill/phone_number_i18n.cc (right):

http://codereview.chromium.org/7044102/diff/8001/chrome/browser/autofill/phon...
chrome/browser/autofill/phone_number_i18n.cc:131: void CachePhone(const
string16& key, const autofill_i18n::PhoneObject& n);
On 2011/06/18 00:47:14, Ilya Sherman wrote:
> Why not combine these two methods together, so that the logic of "cache if
> unavailable" is encapsulated in the cache object rather than exposed to the
> client?

Specifically, I did it so cache will be completely separated from actual phone
processing - the cache is just a cache, it does not call a single libphonenumber
function.

[Ilya Sherman, 2011-06-20 23:55:29]

On 2011/06/20 22:59:11, GeorgeY wrote:
> TEST: AutofillManagerTest.DeterminePossibleFieldTypesForUpload
> Average of five runs, rounded to the nearest ms:
>                  with cache without cache difference
> 50 profiles      589        911           +55%
> 500 profiles     4116       7068          +72%

As discussed offline, let's forgo this additional caching for now -- the gains
don't seem to be high enough, especially since this latency will be on the DB
thread rather than on the UI thread.

[GeorgeY, 2011-06-21 00:19:16]

On 2011/06/20 23:55:29, Ilya Sherman wrote:
> On 2011/06/20 22:59:11, GeorgeY wrote:
> > TEST: AutofillManagerTest.DeterminePossibleFieldTypesForUpload
> > Average of five runs, rounded to the nearest ms:
> >                  with cache without cache difference
> > 50 profiles      589        911           +55%
> > 500 profiles     4116       7068          +72%
> 
> As discussed offline, let's forgo this additional caching for now -- the gains
> don't seem to be high enough, especially since this latency will be on the DB
> thread rather than on the UI thread.

done

[Ilya Sherman, 2011-06-21 00:31:26]

http://codereview.chromium.org/7044102/diff/18008/chrome/browser/autofill/pho...
File chrome/browser/autofill/phone_number_i18n.cc (right):

http://codereview.chromium.org/7044102/diff/18008/chrome/browser/autofill/pho...
chrome/browser/autofill/phone_number_i18n.cc:7: #include <map>
Do you still need this include?

http://codereview.chromium.org/7044102/diff/18008/chrome/browser/autofill/pho...
File chrome/browser/autofill/phone_number_i18n.h (right):

http://codereview.chromium.org/7044102/diff/18008/chrome/browser/autofill/pho...
chrome/browser/autofill/phone_number_i18n.h:113: PhoneMatch ComparePhones(const
PhoneObject& phone) const;
nit: Are there any clients for the second version of this function?

[GeorgeY, 2011-06-21 23:24:44]

done

http://codereview.chromium.org/7044102/diff/18008/chrome/browser/autofill/pho...
File chrome/browser/autofill/phone_number_i18n.cc (right):

http://codereview.chromium.org/7044102/diff/18008/chrome/browser/autofill/pho...
chrome/browser/autofill/phone_number_i18n.cc:7: #include <map>
On 2011/06/21 00:31:26, Ilya Sherman wrote:
> Do you still need this include?

no, removed

http://codereview.chromium.org/7044102/diff/18008/chrome/browser/autofill/pho...
File chrome/browser/autofill/phone_number_i18n.h (right):

http://codereview.chromium.org/7044102/diff/18008/chrome/browser/autofill/pho...
chrome/browser/autofill/phone_number_i18n.h:113: PhoneMatch ComparePhones(const
PhoneObject& phone) const;
On 2011/06/21 00:31:26, Ilya Sherman wrote:
> nit: Are there any clients for the second version of this function?

Not, after recent changes - removed

[Ilya Sherman, 2011-06-21 23:46:12]

LGTM, thanks.

http://codereview.chromium.org/7044102/diff/18011/chrome/browser/autofill/pho...
File chrome/browser/autofill/phone_number_i18n.h (right):

http://codereview.chromium.org/7044102/diff/18011/chrome/browser/autofill/pho...
chrome/browser/autofill/phone_number_i18n.h:108: std::string GetLocale() const {
return locale_; }
nit: Why did you inline just one of these four? ;)

http://codereview.chromium.org/7044102/diff/18011/chrome/browser/autofill/pho...
chrome/browser/autofill/phone_number_i18n.h:116: bool ValidNumber() const {
return i18n_number_ != NULL; }
nit: Perhaps "IsValidNumber" rather than "ValidNumber"?

[dhollowa, 2011-06-21 23:53:02]

LGTM.

[GeorgeY, 2011-06-22 23:43:43]

http://codereview.chromium.org/7044102/diff/18011/chrome/browser/autofill/pho...
File chrome/browser/autofill/phone_number_i18n.h (right):

http://codereview.chromium.org/7044102/diff/18011/chrome/browser/autofill/pho...
chrome/browser/autofill/phone_number_i18n.h:108: std::string GetLocale() const {
return locale_; }
On 2011/06/21 23:46:12, Ilya Sherman wrote:
> nit: Why did you inline just one of these four? ;)

Initially I had lazy initialization of the other members, so the other functions
did some processing, this one was *never* intended to be anything other than
accessor.

http://codereview.chromium.org/7044102/diff/18011/chrome/browser/autofill/pho...
chrome/browser/autofill/phone_number_i18n.h:116: bool ValidNumber() const {
return i18n_number_ != NULL; }
On 2011/06/21 23:46:12, Ilya Sherman wrote:
> nit: Perhaps "IsValidNumber" rather than "ValidNumber"?

Sure