[date] Add a cache for timezone names to DateCache

[date] Add a cache for timezone names to DateCache

To speed up Date.prototype.toString(), this patch adds a cache in
the DateCache for the string short name representing the time zone.
Because time zones in a particular location just have two short names
(for DST and standard time), and the DateCache already understands
whether a time is in DST or not, it is possible to keep the result
of OS::LocalTimezone around and select between the two based on
whether the time is DST or not.

In local microbenchmarks (calling Date.prototype.toString() in a
loop), I observed a 6-10% speedup with this patch. In the browser,
the speedup may be even greater as the system call needs to do
some extra work to break out of the sandbox. I don't think the
microbenchmark is extremely unrealistic; in any real program which
calls Date.prototype.toString() multiple times, the cache should
hit almost all of the time, as time zone changes are rare.

The proximate motivation for this patch was to enable ICU as a
backend for timezone information, which is drafted at
https://codereview.chromium.org/2724373002/
The ICU implementation of OS::LocalTimezone is even slower than
the system call one, but this patch makes their performance
indistinguishable on the microbenchmark.

In the tz database, many timezones actually do have a number of different
historical names. For example, America/Anchorage went through a number of
changes, from AST to AHST to YST to AKST. However, both ICU and the
Linux OS interfaces just report the modern timezone name in tests
for the appropriate timezone name, even for historical times. I can
see why this would be:
- For ICU, CLDR only has two short names in the data file: the one for
  dst and non-dst
- For Linux, the timezone names do seem to make it into the
  /etc/localtime file. However, glibc assumes there are only two relevant
  names and selects between them, as you can see in its implementation
  of localtime_r:
  http://bazaar.launchpad.net/~vcs-imports/glibc/master/view/head:/time/tzset.c#L573
So, this cache should be valid until we switch to a more accurate source
of short timezone names.

BUG=v8:6031
Review-Url: https://codereview.chromium.org/2726253002
Cr-Commit-Position: refs/heads/master@{#43730}
Committed: https://chromium.googlesource.com/v8/v8/+/7e87f446258700aaa04d8760271effba9805e89b

[Dan Ehrenberg, 2017-03-02 20:13:10]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-02 20:13:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Dan Ehrenberg, 2017-03-02 20:15:46]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-02 20:15:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Dan Ehrenberg, 2017-03-02 20:23:28]

Description was changed from

==========
[date] Add a cache for timezone names to DateCache

To speed up Date.prototype.toString(), this patch adds a cache in
the DateCache for the string short name representing the time zone.
Because time zones in a particular location just have two short names
(for DST and standard time), and the DateCache already understands
whether a time is in DST or not, it is possible to keep the result
of OS::LocalTimezone around and select between the two based on
whether the time is DST or not.

In local microbenchmarks (calling Date.prototype.toString() in a
loop), I observed a 10% speedup with this patch. In the browser,
the speedup may be even greater as the system call needs to do
some extra work to break out of the sandbox. I don't think the
microbenchmark is extremely unrealistic; in any real program which
calls Date.prototype.toString() multiple times, the cache should
hit almost all of the time, as time zone changes are rare.

The proximate motivation for this patch was to enable ICU as a
backend for timezone information, which is drafted at
https://codereview.chromium.org/2724373002/
The ICU implementation of OS::LocalTimezone is even slower than
the system call one, but this patch makes their performance
indistinguishable on the microbenchmark.

BUG=v8:6031
==========

to

==========
[date] Add a cache for timezone names to DateCache

To speed up Date.prototype.toString(), this patch adds a cache in
the DateCache for the string short name representing the time zone.
Because time zones in a particular location just have two short names
(for DST and standard time), and the DateCache already understands
whether a time is in DST or not, it is possible to keep the result
of OS::LocalTimezone around and select between the two based on
whether the time is DST or not.

In local microbenchmarks (calling Date.prototype.toString() in a
loop), I observed a 6-10% speedup with this patch. In the browser,
the speedup may be even greater as the system call needs to do
some extra work to break out of the sandbox. I don't think the
microbenchmark is extremely unrealistic; in any real program which
calls Date.prototype.toString() multiple times, the cache should
hit almost all of the time, as time zone changes are rare.

The proximate motivation for this patch was to enable ICU as a
backend for timezone information, which is drafted at
https://codereview.chromium.org/2724373002/
The ICU implementation of OS::LocalTimezone is even slower than
the system call one, but this patch makes their performance
indistinguishable on the microbenchmark.

BUG=v8:6031
==========

[commit-bot: I haz the power, 2017-03-02 20:47:15]

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

[commit-bot: I haz the power, 2017-03-02 20:47:16]

Dry run: This issue passed the CQ dry run.

[Dan Ehrenberg, 2017-03-02 21:20:31]

littledan@chromium.org changed reviewers:
+ ulan@chromium.org

[jgruber, 2017-03-03 08:09:57]

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

[jgruber, 2017-03-03 08:09:57]

https://codereview.chromium.org/2726253002/diff/20001/src/date.h
File src/date.h (right):

https://codereview.chromium.org/2726253002/diff/20001/src/date.h#newcode97
src/date.h:97: const char** name = is_dst ? &dst_tz_name_ : &tz_name_;
Nit: Can you use plain 'const char* name' instead of 'const char**'?

[Dan Ehrenberg, 2017-03-03 12:57:45]

https://codereview.chromium.org/2726253002/diff/20001/src/date.h
File src/date.h (right):

https://codereview.chromium.org/2726253002/diff/20001/src/date.h#newcode97
src/date.h:97: const char** name = is_dst ? &dst_tz_name_ : &tz_name_;
On 2017/03/03 08:09:57, jgruber wrote:
> Nit: Can you use plain 'const char* name' instead of 'const char**'?

I don't think so, because then I don't see how I could write to it on line 99
and have that affect the underlying object. I could use a C++ reference, but
non-const references are discouraged in the Google style guide (though I believe
they're found all over Blink).

[jgruber, 2017-03-03 13:02:34]

https://codereview.chromium.org/2726253002/diff/20001/src/date.h
File src/date.h (right):

https://codereview.chromium.org/2726253002/diff/20001/src/date.h#newcode97
src/date.h:97: const char** name = is_dst ? &dst_tz_name_ : &tz_name_;
On 2017/03/03 12:57:44, Dan Ehrenberg wrote:
> On 2017/03/03 08:09:57, jgruber wrote:
> > Nit: Can you use plain 'const char* name' instead of 'const char**'?
> 
> I don't think so, because then I don't see how I could write to it on line 99
> and have that affect the underlying object. I could use a C++ reference, but
> non-const references are discouraged in the Google style guide (though I
believe
> they're found all over Blink).

Ah, gotcha. sgtm.

[Dan Ehrenberg, 2017-03-03 14:16:11]

Description was changed from

==========
[date] Add a cache for timezone names to DateCache

To speed up Date.prototype.toString(), this patch adds a cache in
the DateCache for the string short name representing the time zone.
Because time zones in a particular location just have two short names
(for DST and standard time), and the DateCache already understands
whether a time is in DST or not, it is possible to keep the result
of OS::LocalTimezone around and select between the two based on
whether the time is DST or not.

In local microbenchmarks (calling Date.prototype.toString() in a
loop), I observed a 6-10% speedup with this patch. In the browser,
the speedup may be even greater as the system call needs to do
some extra work to break out of the sandbox. I don't think the
microbenchmark is extremely unrealistic; in any real program which
calls Date.prototype.toString() multiple times, the cache should
hit almost all of the time, as time zone changes are rare.

The proximate motivation for this patch was to enable ICU as a
backend for timezone information, which is drafted at
https://codereview.chromium.org/2724373002/
The ICU implementation of OS::LocalTimezone is even slower than
the system call one, but this patch makes their performance
indistinguishable on the microbenchmark.

BUG=v8:6031
==========

to

==========
[date] Add a cache for timezone names to DateCache

To speed up Date.prototype.toString(), this patch adds a cache in
the DateCache for the string short name representing the time zone.
Because time zones in a particular location just have two short names
(for DST and standard time), and the DateCache already understands
whether a time is in DST or not, it is possible to keep the result
of OS::LocalTimezone around and select between the two based on
whether the time is DST or not.

In local microbenchmarks (calling Date.prototype.toString() in a
loop), I observed a 6-10% speedup with this patch. In the browser,
the speedup may be even greater as the system call needs to do
some extra work to break out of the sandbox. I don't think the
microbenchmark is extremely unrealistic; in any real program which
calls Date.prototype.toString() multiple times, the cache should
hit almost all of the time, as time zone changes are rare.

The proximate motivation for this patch was to enable ICU as a
backend for timezone information, which is drafted at
https://codereview.chromium.org/2724373002/
The ICU implementation of OS::LocalTimezone is even slower than
the system call one, but this patch makes their performance
indistinguishable on the microbenchmark.

In the tz database, many timezones actually do have a number of different
historical names. For example, America/Anchorage went through a number of
changes, from AST to AHST to YST to AKST. However, both ICU and the
Linux OS interfaces just report the modern timezone name in tests
for the appropriate timezone name, even for historical times. I can
see why this would be:
- For ICU, CLDR only has two short names in the data file: the one for
  dst and non-dst
- For Linux, the timezone names do seem to make it into the
  /etc/localtime file. However, glibc assumes there are only two relevant
  names and selects between them, as you can see in its implementation
  of localtime_r:
 
http://bazaar.launchpad.net/~vcs-imports/glibc/master/view/head:/time/tzset.c...
So, this cache should be valid until we switch to a more accurate source
of short timezone names.

BUG=v8:6031
==========

[jungshik at Google, 2017-03-09 23:33:52]

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

[jungshik at Google, 2017-03-09 23:33:53]

As mentioned in another CL, I'm really tempted to move away from the current
output format for Date.toString() which is different on different platforms.  On
Windows, we show a rather long descriptive and *localized* name enclosed by
parens. On Linux, we show 'cryptic' 3-4 letters long abbreviations.

[Dan Ehrenberg, 2017-03-10 13:08:58]

On 2017/03/09 23:33:53, jungshik at Google wrote:
> As mentioned in another CL, I'm really tempted to move away from the current
> output format for Date.toString() which is different on different platforms. 
On
> Windows, we show a rather long descriptive and *localized* name enclosed by
> parens. On Linux, we show 'cryptic' 3-4 letters long abbreviations.

This patch doesn't change where we get the names from; it just caches them.
Regardless of which choice we make for improving the source of the names, this
can lead to a performance improvement, even if it's only by preventing redundant
checks about whether we're currently in DST. I opened a bug to discuss what we
should do about timezone names in
https://bugs.chromium.org/p/v8/issues/detail?id=6076 .

[Dan Ehrenberg, 2017-03-10 13:09:13]

littledan@chromium.org changed reviewers:
+ yangguo@chromium.org

[ulan, 2017-03-10 17:46:29]

LGTM, but please wait for jgruber@.

[jungshik at Google, 2017-03-10 20:34:35]

On 2017/03/10 13:08:58, Dan Ehrenberg wrote:
> On 2017/03/09 23:33:53, jungshik at Google wrote:
> > As mentioned in another CL, I'm really tempted to move away from the current
> > output format for Date.toString() which is different on different platforms.

> On
> > Windows, we show a rather long descriptive and *localized* name enclosed by
> > parens. On Linux, we show 'cryptic' 3-4 letters long abbreviations.
> 
> This patch doesn't change where we get the names from; it just caches them.
> Regardless of which choice we make for improving the source of the names, this
> can lead to a performance improvement, even if it's only by preventing
redundant
> checks about whether we're currently in DST. I opened a bug to discuss what we
> should do about timezone names in
> https://bugs.chromium.org/p/v8/issues/detail?id=6076 .

Yeah, I realized that after writing my comment in a hurry. Thanks. LGTM, too.

[jgruber, 2017-03-11 08:48:47]

lgtm

[Dan Ehrenberg, 2017-03-11 08:50:15]

The CQ bit was checked by littledan@chromium.org

[commit-bot: I haz the power, 2017-03-11 08:50:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-11 08:51:56]

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

[commit-bot: I haz the power, 2017-03-11 08:51:57]

Try jobs failed on following builders:
  v8_android_arm_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...)
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/22320)
  v8_linux64_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_verify_csa_rel_n...)
  v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)
  v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/22347)
  v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  v8_linux_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_verify_csa_rel_ng/...)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/36393)
  v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/24005)

[Dan Ehrenberg, 2017-03-11 21:10:20]

The CQ bit was checked by littledan@chromium.org

[Dan Ehrenberg, 2017-03-11 21:10:20]

The patchset sent to the CQ was uploaded after l-g-t-m from ulan@chromium.org,
jshin@chromium.org, jgruber@chromium.org
Link to the patchset: https://codereview.chromium.org/2726253002/#ps40001
(title: "Rebase")

[commit-bot: I haz the power, 2017-03-11 21:10:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-11 22:49:37]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1489266620224560,
"parent_rev": "244d014f8a94a172c6c657a29e0eb3be04ef2178", "commit_rev":
"7e87f446258700aaa04d8760271effba9805e89b"}

[commit-bot: I haz the power, 2017-03-11 22:49:43]

Description was changed from

==========
[date] Add a cache for timezone names to DateCache

To speed up Date.prototype.toString(), this patch adds a cache in
the DateCache for the string short name representing the time zone.
Because time zones in a particular location just have two short names
(for DST and standard time), and the DateCache already understands
whether a time is in DST or not, it is possible to keep the result
of OS::LocalTimezone around and select between the two based on
whether the time is DST or not.

In local microbenchmarks (calling Date.prototype.toString() in a
loop), I observed a 6-10% speedup with this patch. In the browser,
the speedup may be even greater as the system call needs to do
some extra work to break out of the sandbox. I don't think the
microbenchmark is extremely unrealistic; in any real program which
calls Date.prototype.toString() multiple times, the cache should
hit almost all of the time, as time zone changes are rare.

The proximate motivation for this patch was to enable ICU as a
backend for timezone information, which is drafted at
https://codereview.chromium.org/2724373002/
The ICU implementation of OS::LocalTimezone is even slower than
the system call one, but this patch makes their performance
indistinguishable on the microbenchmark.

In the tz database, many timezones actually do have a number of different
historical names. For example, America/Anchorage went through a number of
changes, from AST to AHST to YST to AKST. However, both ICU and the
Linux OS interfaces just report the modern timezone name in tests
for the appropriate timezone name, even for historical times. I can
see why this would be:
- For ICU, CLDR only has two short names in the data file: the one for
  dst and non-dst
- For Linux, the timezone names do seem to make it into the
  /etc/localtime file. However, glibc assumes there are only two relevant
  names and selects between them, as you can see in its implementation
  of localtime_r:
 
http://bazaar.launchpad.net/~vcs-imports/glibc/master/view/head:/time/tzset.c...
So, this cache should be valid until we switch to a more accurate source
of short timezone names.

BUG=v8:6031
==========

to

==========
[date] Add a cache for timezone names to DateCache

To speed up Date.prototype.toString(), this patch adds a cache in
the DateCache for the string short name representing the time zone.
Because time zones in a particular location just have two short names
(for DST and standard time), and the DateCache already understands
whether a time is in DST or not, it is possible to keep the result
of OS::LocalTimezone around and select between the two based on
whether the time is DST or not.

In local microbenchmarks (calling Date.prototype.toString() in a
loop), I observed a 6-10% speedup with this patch. In the browser,
the speedup may be even greater as the system call needs to do
some extra work to break out of the sandbox. I don't think the
microbenchmark is extremely unrealistic; in any real program which
calls Date.prototype.toString() multiple times, the cache should
hit almost all of the time, as time zone changes are rare.

The proximate motivation for this patch was to enable ICU as a
backend for timezone information, which is drafted at
https://codereview.chromium.org/2724373002/
The ICU implementation of OS::LocalTimezone is even slower than
the system call one, but this patch makes their performance
indistinguishable on the microbenchmark.

In the tz database, many timezones actually do have a number of different
historical names. For example, America/Anchorage went through a number of
changes, from AST to AHST to YST to AKST. However, both ICU and the
Linux OS interfaces just report the modern timezone name in tests
for the appropriate timezone name, even for historical times. I can
see why this would be:
- For ICU, CLDR only has two short names in the data file: the one for
  dst and non-dst
- For Linux, the timezone names do seem to make it into the
  /etc/localtime file. However, glibc assumes there are only two relevant
  names and selects between them, as you can see in its implementation
  of localtime_r:
 
http://bazaar.launchpad.net/~vcs-imports/glibc/master/view/head:/time/tzset.c...
So, this cache should be valid until we switch to a more accurate source
of short timezone names.

BUG=v8:6031

Review-Url: https://codereview.chromium.org/2726253002
Cr-Commit-Position: refs/heads/master@{#43730}
Committed:
https://chromium.googlesource.com/v8/v8/+/7e87f446258700aaa04d8760271effba980...
==========

[commit-bot: I haz the power, 2017-03-11 22:49:44]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/v8/v8/+/7e87f446258700aaa04d8760271effba980...