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

chrome/browser/autofill/phone_number_i18n.cc

Index: chrome/browser/autofill/phone_number_i18n.cc
===================================================================
--- chrome/browser/autofill/phone_number_i18n.cc	(revision 88362)
+++ chrome/browser/autofill/phone_number_i18n.cc	(working copy)
@@ -4,6 +4,8 @@
 
 #include "chrome/browser/autofill/phone_number_i18n.h"
 
+#include <map>
+
 #include "base/basictypes.h"
 #include "base/logging.h"
 #include "base/stringprintf.h"
@@ -38,41 +40,16 @@
   return i18n::phonenumbers::PhoneNumberUtil::NATIONAL;
 }
 
-}  // namespace
-
-namespace autofill_i18n {
-
-string16 NormalizePhoneNumber(const string16& value,
-                              std::string const& locale) {
-  string16 number;
-  string16 city_code;
-  string16 country_code;
-  string16 result;
-  // Full number - parse it, split it and re-combine into canonical form.
-  if (!ParsePhoneNumber(value, locale, &country_code, &city_code, &number))
-    return string16();  // Parsing failed - do not store phone.
-  if (!autofill_i18n::ConstructPhoneNumber(
-          country_code, city_code, number,
-          locale,
-          (country_code.empty() ?
-              autofill_i18n::NATIONAL : autofill_i18n::INTERNATIONAL),
-          &result)) {
-    // Reconstruction failed - do not store phone.
-    return string16();
-  }
-  std::string result_utf8(UTF16ToUTF8(result));
-  i18n::phonenumbers::PhoneNumberUtil::NormalizeDigitsOnly(&result_utf8);
-  return UTF8ToUTF16(result_utf8);
-}
-
-bool ParsePhoneNumber(const string16& value,
-                      const std::string& locale,
-                      string16* country_code,
-                      string16* city_code,
-                      string16* number) {
+bool ParsePhoneNumberInternal(const string16& value,
+                              const std::string& locale,
+                              string16* country_code,
+                              string16* city_code,
+                              string16* number,
+                              i18n::phonenumbers::PhoneNumber* i18n_number) {
   DCHECK(number);
   DCHECK(city_code);
   DCHECK(country_code);
+  DCHECK(i18n_number);
 
   number->clear();
   city_code->clear();
@@ -81,38 +58,37 @@
   std::string number_text(UTF16ToUTF8(value));
 
   // Parse phone number based on the locale.
-  i18n::phonenumbers::PhoneNumber i18n_number;
   i18n::phonenumbers::PhoneNumberUtil* phone_util =
       i18n::phonenumbers::PhoneNumberUtil::GetInstance();
   DCHECK(phone_util);
 
   if (phone_util->Parse(number_text, SanitizeLocaleCode(locale).c_str(),
-                        &i18n_number) !=
+                        i18n_number) !=
       i18n::phonenumbers::PhoneNumberUtil::NO_PARSING_ERROR) {
     return false;
   }
 
   i18n::phonenumbers::PhoneNumberUtil::ValidationResult validation =
-      phone_util->IsPossibleNumberWithReason(i18n_number);
+      phone_util->IsPossibleNumberWithReason(*i18n_number);
   if (validation != i18n::phonenumbers::PhoneNumberUtil::IS_POSSIBLE)
     return false;
 
   // This verifies that number has a valid area code (that in some cases could
   // be empty) for parsed country code. Also verifies that this is a valid
   // number (in US 1234567 is not valid, because numbers do not start with 1).
-  if (!phone_util->IsValidNumber(i18n_number))
+  if (!phone_util->IsValidNumber(*i18n_number))
     return false;
 
   std::string national_significant_number;
-  phone_util->GetNationalSignificantNumber(i18n_number,
+  phone_util->GetNationalSignificantNumber(*i18n_number,
                                            &national_significant_number);
 
   std::string area_code;
   std::string subscriber_number;
 
-  int area_length = phone_util->GetLengthOfGeographicalAreaCode(i18n_number);
+  int area_length = phone_util->GetLengthOfGeographicalAreaCode(*i18n_number);
   int destination_length =
-      phone_util->GetLengthOfNationalDestinationCode(i18n_number);
+      phone_util->GetLengthOfNationalDestinationCode(*i18n_number);
   // Some phones have a destination code in lieu of area code: mobile operators
   // in Europe, toll and toll-free numbers in USA, etc. From our point of view
   // these two types of codes are the same.
@@ -132,9 +108,9 @@
   string16 normalized_number(UTF8ToUTF16(number_text));
   // Check if parsed number has country code and it was not inferred from the
   // locale.
-  if (i18n_number.has_country_code()) {
-    *country_code = UTF8ToUTF16(base::StringPrintf("%d",
-                                                   i18n_number.country_code()));
+  if (i18n_number->has_country_code()) {
+    *country_code = UTF8ToUTF16(
+         base::StringPrintf("%d", i18n_number->country_code()));
     if (normalized_number.length() <= national_significant_number.length() &&
         (normalized_number.length() < country_code->length() ||
         normalized_number.compare(0, country_code->length(), *country_code))) {
@@ -145,6 +121,43 @@
   return true;
 }
 
+}  // namespace
+
+namespace autofill_i18n {
+
+string16 NormalizePhoneNumber(const string16& value,
+                              std::string const& locale) {
+  string16 number;
+  string16 city_code;
+  string16 country_code;
+  string16 result;
+  // Full number - parse it, split it and re-combine into canonical form.
+  if (!ParsePhoneNumber(value, locale, &country_code, &city_code, &number))
+    return string16();  // Parsing failed - do not store phone.
+  if (!autofill_i18n::ConstructPhoneNumber(
+          country_code, city_code, number,
+          locale,
+          (country_code.empty() ?
+              autofill_i18n::NATIONAL : autofill_i18n::INTERNATIONAL),
+          &result)) {
+    // Reconstruction failed - do not store phone.
+    return string16();
+  }
+  std::string result_utf8(UTF16ToUTF8(result));
+  i18n::phonenumbers::PhoneNumberUtil::NormalizeDigitsOnly(&result_utf8);
+  return UTF8ToUTF16(result_utf8);
+}
+
+bool ParsePhoneNumber(const string16& value,
+                      const std::string& locale,
+                      string16* country_code,
+                      string16* city_code,
+                      string16* number) {
+  i18n::phonenumbers::PhoneNumber i18n_number;
+  return ParsePhoneNumberInternal(value, locale, country_code, city_code,
+                                  number, &i18n_number);
+}
+
 bool ConstructPhoneNumber(const string16& country_code,
                           const string16& city_code,
                           const string16& number,
@@ -265,5 +278,124 @@
   return ComparePhones(number_a, number_b, country_code) == PHONES_EQUAL;
 }
 
+PhoneObject::PhoneObject(const string16& number, const std::string& locale)
+    : locale_(SanitizeLocaleCode(locale)),
+      i18n_number_(NULL) {
+  scoped_ptr<i18n::phonenumbers::PhoneNumber>
+    i18n_number(new i18n::phonenumbers::PhoneNumber);
+  if (ParsePhoneNumberInternal(number, locale_, &country_code_, &city_code_,
+                               &number_, i18n_number.get())) {
+    i18n_number_ = i18n_number.release();
+  } else {
+    whole_number_ = number;
+  }

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

nit: The else stmt here makes the code a little bit more confusing to read.  How
about moving this logic into GetWholeNumber()?

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

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

+}
+
+PhoneObject::PhoneObject(const PhoneObject& other)
+    : i18n_number_(NULL) {
+  *this = other;
+}
+
+PhoneObject::PhoneObject()
+    : i18n_number_(NULL) {
+}
+
+PhoneObject::~PhoneObject() {
+  Clear();
+}
+
+string16 PhoneObject::GetCountryCode() const {
+  return country_code_;
+}
+
+string16 PhoneObject::GetCityCode() const {
+  return city_code_;
+}
+
+string16 PhoneObject::GetNumber() const {
+  return number_;
+}
+
+string16 PhoneObject::GetWholeNumber() const {
+  if (i18n_number_ && whole_number_.empty()) {
+    i18n::phonenumbers::PhoneNumberUtil::PhoneNumberFormat format =
+        i18n::phonenumbers::PhoneNumberUtil::INTERNATIONAL;
+    if (country_code_.empty())
+      format = i18n::phonenumbers::PhoneNumberUtil::NATIONAL;
+
+    std::string formatted_number;
+    i18n::phonenumbers::PhoneNumberUtil* phone_util =
+        i18n::phonenumbers::PhoneNumberUtil::GetInstance();
+    phone_util->Format(*i18n_number_, format, &formatted_number);
+    i18n::phonenumbers::PhoneNumberUtil::NormalizeDigitsOnly(&formatted_number);
+    whole_number_ = UTF8ToUTF16(formatted_number);
+  }
+  return whole_number_;
+}
+
+PhoneMatch PhoneObject::ComparePhones(const string16& phone) const {
+  const size_t kMaxCacheSize = 1024;
+  static std::map<string16, PhoneObject> cached_objects;
+  // Remove last element from cache to prevent unlimited grow.
+  if (cached_objects.size() > kMaxCacheSize) {
+    std::map<string16, PhoneObject>::iterator it = cached_objects.end();
+    --it;
+    cached_objects.erase(it);
+  }
+  string16 key(ASCIIToUTF16(locale_));
+  key.append(phone);
+  std::map<string16, PhoneObject>::const_iterator it = cached_objects.find(key);
+  if (it == cached_objects.end()) {
+    // Add it to the cache.
+    it = cached_objects.insert(
+        std::pair<string16, PhoneObject>(key,
+            PhoneObject(phone, locale_))).first;
+    DCHECK(it != cached_objects.end());
+  }
+  return ComparePhones(it->second);
+}
+
+PhoneMatch PhoneObject::ComparePhones(const PhoneObject& phone) const {
+  if (!i18n_number_ || !phone.i18n_number_) {
+    if (GetWholeNumber().empty())
+      return PHONES_NOT_EQUAL;
+    return (GetWholeNumber() == phone.GetWholeNumber()) ? PHONES_EQUAL :
+                                                          PHONES_NOT_EQUAL;
+  }
+
+  i18n::phonenumbers::PhoneNumberUtil* phone_util =
+      i18n::phonenumbers::PhoneNumberUtil::GetInstance();
+  switch (phone_util->IsNumberMatch(*i18n_number_, *(phone.i18n_number_))) {
+    case i18n::phonenumbers::PhoneNumberUtil::INVALID_NUMBER:
+    case i18n::phonenumbers::PhoneNumberUtil::NO_MATCH:
+      return PHONES_NOT_EQUAL;
+    case i18n::phonenumbers::PhoneNumberUtil::SHORT_NSN_MATCH:
+      return PHONES_SUBMATCH;
+    case i18n::phonenumbers::PhoneNumberUtil::NSN_MATCH:
+    case i18n::phonenumbers::PhoneNumberUtil::EXACT_MATCH:
+      return PHONES_EQUAL;
+    default:
+      NOTREACHED();
+  }
+  return PHONES_NOT_EQUAL;
+}
+
+PhoneObject& PhoneObject::operator=(const PhoneObject& other) {

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

This is not currently a no-op when this == &other, but it should be.

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

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

+  country_code_ = other.country_code_;
+  city_code_ = other.city_code_;
+  number_ = other.number_;
+  locale_ = other.locale_;
+  Clear();
+  if (other.i18n_number_)
+    i18n_number_ = new i18n::phonenumbers::PhoneNumber(*other.i18n_number_);
+  return *this;
+}
+
+void PhoneObject::Clear() {
+  if (i18n_number_)
+    delete i18n_number_;

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

How about making i18n_number_ be a scoped_ptr<> instead, and using the reset()
method?

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

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.

+  i18n_number_ = NULL;
+}
+
 }  // namespace autofill_i18n