Use a more suitable storage for the timezone table.

Use a more suitable storage for the timezone table.

Creating a runtime table of std::string objects, just to copy those
into a map, is not optimal from a runtime or footprint point of view.

Making the table use raw char pointers shrinks the code and data by 75%,
from 36 KB to 10 KB (clang, Linux, 64 bit content_shell).

clang:
Total change: -27266 bytes
==========================
  2 added, totalling +8448 bytes across 1 sources
  774 removed, totalling -36401 bytes across 2 sources
  1 grown, for a net change of +687 bytes (481 bytes before, 1168 bytes after) across 1 sources

R=estade@chromium.org,mark@chromium.org
BUG=

Committed: https://crrev.com/68f667d7c49c3c09cb084dd2739ce5445130464d
Cr-Commit-Position: refs/heads/master@{#297821}

[Daniel Bratell, 2014-09-15 09:27:57]

bratell@opera.com changed reviewers:
+ estade@chromium.org, jshin@chromium.org

[Daniel Bratell, 2014-09-15 11:01:08]

jshin, could you please take a look at this change from std::string to const
char* in a table? I found it by noticing some strangely large blocks of code.

[Evan Stade, 2014-09-15 17:14:56]

sweet, lgtm (I'm not an owner though)

[Evan Stade, 2014-09-25 00:05:34]

ping jshin

[Mark Mentovai, 2014-10-01 13:05:06]

mark@chromium.org changed reviewers:
+ mark@chromium.org

[Mark Mentovai, 2014-10-01 13:05:07]

At the very least, I think the “value” side of the map should change to const
char*. The other const char* changes are optional.

https://codereview.chromium.org/573623003/diff/1/base/i18n/timezone.cc
File base/i18n/timezone.cc (right):

https://codereview.chromium.org/573623003/diff/1/base/i18n/timezone.cc#newcod...
base/i18n/timezone.cc:463: for (size_t i = 0; i <
ARRAYSIZE_UNSAFE(olson_code_data); ++i) {
This can be normal arraysize now. Same below.

https://codereview.chromium.org/573623003/diff/1/base/i18n/timezone.cc#newcod...
base/i18n/timezone.cc:465: std::string(olson_code_data[i].country_code);
You can make the “value” side of the map store const char* instead of
std::string at least, and avoid these constructions.

If you want to fiddle with a comparator template argument, you can also make the
“key” side use const char*.

You may also be able to bubble these changes through to CountryCodeForTimezone()
and CountryCodeForCurrentTimezone().

[Daniel Bratell, 2014-10-01 19:56:23]

Switched to full const char* -> const char*. It cost ~1 KB in a new map
implementation (sometimes STL makes me really tired) but it should still be a
big win since we don't need to wrap ~1000 strings in std::string object. Further
improvements (using a static sorted table and binary search) are not worth the
maintenance cost I believe.

mark, ptal?

https://codereview.chromium.org/573623003/diff/1/base/i18n/timezone.cc
File base/i18n/timezone.cc (right):

https://codereview.chromium.org/573623003/diff/1/base/i18n/timezone.cc#newcod...
base/i18n/timezone.cc:463: for (size_t i = 0; i <
ARRAYSIZE_UNSAFE(olson_code_data); ++i) {
On 2014/10/01 13:05:07, Mark Mentovai wrote:
> This can be normal arraysize now. Same below.

Done.

https://codereview.chromium.org/573623003/diff/1/base/i18n/timezone.cc#newcod...
base/i18n/timezone.cc:465: std::string(olson_code_data[i].country_code);
On 2014/10/01 13:05:07, Mark Mentovai wrote:
> You can make the “value” side of the map store const char* instead of
> std::string at least, and avoid these constructions.
> 
> If you want to fiddle with a comparator template argument, you can also make
the
> “key” side use const char*.
> 
> You may also be able to bubble these changes through to
CountryCodeForTimezone()
> and CountryCodeForCurrentTimezone().

Done.

[Mark Mentovai, 2014-10-01 20:03:52]

LGTM. Nice.

https://codereview.chromium.org/573623003/diff/60001/base/i18n/timezone.cc
File base/i18n/timezone.cc (right):

https://codereview.chromium.org/573623003/diff/60001/base/i18n/timezone.cc#ne...
base/i18n/timezone.cc:8: #include <map>
Chrome code puts a blank line between C and C++ system headers.

https://codereview.chromium.org/573623003/diff/60001/base/i18n/timezone.cc#ne...
base/i18n/timezone.cc:606: std::string CountryCodeForCurrentTimezone() {
This could maybe return a const char* now too, but if all of the callers are
just going to construct an std::string anyway, there’s not much of a point.

[Daniel Bratell, 2014-10-02 06:23:47]

https://codereview.chromium.org/573623003/diff/60001/base/i18n/timezone.cc
File base/i18n/timezone.cc (right):

https://codereview.chromium.org/573623003/diff/60001/base/i18n/timezone.cc#ne...
base/i18n/timezone.cc:8: #include <map>
On 2014/10/01 20:03:52, Mark Mentovai wrote:
> Chrome code puts a blank line between C and C++ system headers.

Done.

https://codereview.chromium.org/573623003/diff/60001/base/i18n/timezone.cc#ne...
base/i18n/timezone.cc:606: std::string CountryCodeForCurrentTimezone() {
On 2014/10/01 20:03:52, Mark Mentovai wrote:
> This could maybe return a const char* now too, but if all of the callers are
> just going to construct an std::string anyway, there’s not much of a point.

The (few) callers do indeed work with std::string. I wouldn't mind pure string
pointers to be used more but it's better to create std::string at one place than
at many places so this will be fine.

[Daniel Bratell, 2014-10-02 06:24:48]

The CQ bit was checked by bratell@opera.com

[commit-bot: I haz the power, 2014-10-02 06:25:08]

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

[Daniel Bratell, 2014-10-02 06:28:41]

The CQ bit was checked by bratell@opera.com

[commit-bot: I haz the power, 2014-10-02 06:28:49]

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

[commit-bot: I haz the power, 2014-10-02 06:35:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-02 06:35:38]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Daniel Bratell, 2014-10-02 08:43:53]

The CQ bit was checked by bratell@opera.com

[commit-bot: I haz the power, 2014-10-02 08:44:39]

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

[commit-bot: I haz the power, 2014-10-02 08:50:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-02 08:50:44]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[Daniel Bratell, 2014-10-02 09:02:35]

The CQ bit was checked by bratell@opera.com

[commit-bot: I haz the power, 2014-10-02 09:03:46]

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

[commit-bot: I haz the power, 2014-10-02 09:12:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-02 09:12:35]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[Daniel Bratell, 2014-10-02 13:01:38]

The CQ bit was checked by bratell@opera.com

[commit-bot: I haz the power, 2014-10-02 13:01:59]

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

[commit-bot: I haz the power, 2014-10-02 13:16:49]

Committed patchset #5 (id:80001) as dee1aeb60afa4a078a0719d9e5c2c8b2273f87f4

[commit-bot: I haz the power, 2014-10-02 13:17:28]

Patchset 5 (id:??) landed as
https://crrev.com/68f667d7c49c3c09cb084dd2739ce5445130464d
Cr-Commit-Position: refs/heads/master@{#297821}