Invalidate defaultObjects if timezone changes

Invalidate defaultObjects if timezone changes

In case of calling 'toLocaleString', 'toLocaleTimeString' and
'toLocaleDateString' functions of 'Date' with empty 'locales' and
'options', DateTimeFormat is cached inside 'defaultObjects'.
If we change the timezone the cache is not invalidated.

BUG=v8:5022
TEST=cctest:DateCacheVersion. See the bug
CQ_INCLUDE_TRYBOTS=tryserver.v8:v8_linux_noi18n_rel_ng

Committed: https://crrev.com/7afd712ae01cb40aa233ab80dad1b69a8b72a76c
Cr-Commit-Position: refs/heads/master@{#36420}

[pgorszkowski, 2016-05-18 10:55:07]

Description was changed from

==========
Invalidate defaultObjects if timezone changes

In case of calling 'toLocaleString', 'toLocaleTimeString' and
'toLocaleDateString' functions of 'Date' with empty 'locales' and
'options', DateTimeFormat is cached inside 'defaultObjects'.
If we change the timezone the cache is not invalidated.

BUG=5022

TEST=see: https://bugs.chromium.org/p/v8/issues/detail?id=5022
for more details.
==========

to

==========
Invalidate defaultObjects if timezone changes

In case of calling 'toLocaleString', 'toLocaleTimeString' and
'toLocaleDateString' functions of 'Date' with empty 'locales' and
'options', DateTimeFormat is cached inside 'defaultObjects'.
If we change the timezone the cache is not invalidated.

BUG=https://bugs.chromium.org/p/v8/issues/detail?id=5022

TEST=see: https://bugs.chromium.org/p/v8/issues/detail?id=5022
for more details.
==========

[Dan Ehrenberg, 2016-05-18 14:48:34]

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

[commit-bot: I haz the power, 2016-05-18 14:48:41]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1985423003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1985423003/40001

[Dan Ehrenberg, 2016-05-18 14:48:47]

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

[commit-bot: I haz the power, 2016-05-18 15:22:18]

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

[commit-bot: I haz the power, 2016-05-18 15:22:19]

Dry run: This issue passed the CQ dry run.

[jungshik at Google, 2016-05-18 17:29:42]

Thanks. Actually, I was planning to add something similar to this to my WiP CL
(to fix this issue and the past/future timezone offset issue :
https://bugs.chromium.org/p/v8/issues/detail?id=3547 ). 

This CL can go in first and I'll rebase my CL (
https://codereview.chromium.org/1973183002 )

LGTM

One nit: Please, change BUG= line to 

BUG=v8:5022

[jungshik at Google, 2016-05-18 17:30:13]

Description was changed from

==========
Invalidate defaultObjects if timezone changes

In case of calling 'toLocaleString', 'toLocaleTimeString' and
'toLocaleDateString' functions of 'Date' with empty 'locales' and
'options', DateTimeFormat is cached inside 'defaultObjects'.
If we change the timezone the cache is not invalidated.

BUG=https://bugs.chromium.org/p/v8/issues/detail?id=5022

TEST=see: https://bugs.chromium.org/p/v8/issues/detail?id=5022
for more details.
==========

to

==========
Invalidate defaultObjects if timezone changes

In case of calling 'toLocaleString', 'toLocaleTimeString' and
'toLocaleDateString' functions of 'Date' with empty 'locales' and
'options', DateTimeFormat is cached inside 'defaultObjects'.
If we change the timezone the cache is not invalidated.

BUG=v8:5022

TEST=see: https://bugs.chromium.org/p/v8/issues/detail?id=5022
for more details.
==========

[jungshik at Google, 2016-05-18 17:31:26]

Description was changed from

==========
Invalidate defaultObjects if timezone changes

In case of calling 'toLocaleString', 'toLocaleTimeString' and
'toLocaleDateString' functions of 'Date' with empty 'locales' and
'options', DateTimeFormat is cached inside 'defaultObjects'.
If we change the timezone the cache is not invalidated.

BUG=v8:5022

TEST=see: https://bugs.chromium.org/p/v8/issues/detail?id=5022
for more details.
==========

to

==========
Invalidate defaultObjects if timezone changes

In case of calling 'toLocaleString', 'toLocaleTimeString' and
'toLocaleDateString' functions of 'Date' with empty 'locales' and
'options', DateTimeFormat is cached inside 'defaultObjects'.
If we change the timezone the cache is not invalidated.

BUG=v8:5022
TEST=see the bug
==========

[jungshik at Google, 2016-05-18 17:34:06]

On 2016/05/18 17:29:42, jshin (jungshik at google) wrote:
> Thanks. Actually, I was planning to add something similar to this to my WiP CL
> (to fix this issue and the past/future timezone offset issue :
> https://bugs.chromium.org/p/v8/issues/detail?id=3547 ). 
> 
> This CL can go in first and I'll rebase my CL (
> https://codereview.chromium.org/1973183002 )
> 
> LGTM
> 
> One nit: Please, change BUG= line to 
> 
> BUG=v8:5022

I just did that, myself :-)

[Dan Ehrenberg, 2016-05-18 17:39:57]

Do you have an idea of the performance impact in the normal case where you don't
change time zones?

https://codereview.chromium.org/1985423003/diff/40001/src/runtime/runtime-dat...
File src/runtime/runtime-date.cc (right):

https://codereview.chromium.org/1985423003/diff/40001/src/runtime/runtime-dat...
src/runtime/runtime-date.cc:39: RUNTIME_FUNCTION(Runtime_DateCacheVersion) {
If this is only used by Intl, better to put it in runtime-i18n.cc

https://codereview.chromium.org/1985423003/diff/40001/src/runtime/runtime-dat...
src/runtime/runtime-date.cc:55:
isolate->factory()->NewJSObject(isolate->array_function());
Why is this wrapped in an array, rather than just returning the first element,
if that is all that's used?

[jungshik at Google, 2016-05-18 18:06:31]

On 2016/05/18 17:39:57, Dan Ehrenberg wrote:
> Do you have an idea of the performance impact in the normal case where you
don't
> change time zones?

Presumably, %DateCacheVersion() is fast enough. Note that
Date.prototype.ToString() already does this
kind of invalidation in C++. See
https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/date.cc&l=25
(ResetDateCache)
and
https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/api.cc&l=6146



> 
>
https://codereview.chromium.org/1985423003/diff/40001/src/runtime/runtime-dat...
> File src/runtime/runtime-date.cc (right):
> 
>
https://codereview.chromium.org/1985423003/diff/40001/src/runtime/runtime-dat...
> src/runtime/runtime-date.cc:39: RUNTIME_FUNCTION(Runtime_DateCacheVersion) {
> If this is only used by Intl, better to put it in runtime-i18n.cc
> 
>
https://codereview.chromium.org/1985423003/diff/40001/src/runtime/runtime-dat...
> src/runtime/runtime-date.cc:55:
> isolate->factory()->NewJSObject(isolate->array_function());
> Why is this wrapped in an array, rather than just returning the first element,
> if that is all that's used?

I guess that's taken from another place DateCacheVersion is used. I was
wondering about that when looking at that code. See
https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/api.cc&l=6146

[jungshik at Google, 2016-05-18 18:15:46]

On 2016/05/18 18:06:31, jshin (jungshik at google) wrote:
> On 2016/05/18 17:39:57, Dan Ehrenberg wrote:
> > Do you have an idea of the performance impact in the normal case where you
> don't
> > change time zones?
> 
> Presumably, %DateCacheVersion() is fast enough. Note that
> Date.prototype.ToString() already does this
> kind of invalidation in C++. See
> https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/date.cc&l=25
> (ResetDateCache)
> and
>
https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/api.cc&l=6146


Actually, there's a slightly better way. My CL (
https://codereview.chromium.org/1973183002 ; it checks and update timezone
everytime, though. My next step was to check DateCacheVersion) ) only updates
the timezone instead of 
invaldating the whole ICU DateTimeFormat object. It's a bit expensive to create
a new instance from the ground up. So, what my CL does is just
update the timezone (nothing else is changed). 

We can land this CL and then follow this up by my optimization (with other
comments addressed). 

> 
> > 
> >
>
https://codereview.chromium.org/1985423003/diff/40001/src/runtime/runtime-dat...
> > File src/runtime/runtime-date.cc (right):
> > 
> >
>
https://codereview.chromium.org/1985423003/diff/40001/src/runtime/runtime-dat...
> > src/runtime/runtime-date.cc:39: RUNTIME_FUNCTION(Runtime_DateCacheVersion) {
> > If this is only used by Intl, better to put it in runtime-i18n.cc
> > 
> >
>
https://codereview.chromium.org/1985423003/diff/40001/src/runtime/runtime-dat...
> > src/runtime/runtime-date.cc:55:
> > isolate->factory()->NewJSObject(isolate->array_function());
> > Why is this wrapped in an array, rather than just returning the first
element,
> > if that is all that's used?
> 
> I guess that's taken from another place DateCacheVersion is used. I was
> wondering about that when looking at that code. See
>
https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/api.cc&l=6146

[jungshik at Google, 2016-05-18 19:59:24]

On 2016/05/18 18:15:46, jshin (jungshik at google) wrote:
> On 2016/05/18 18:06:31, jshin (jungshik at google) wrote:
> > On 2016/05/18 17:39:57, Dan Ehrenberg wrote:
> > > Do you have an idea of the performance impact in the normal case where you
> > don't
> > > change time zones?
> > 
> > Presumably, %DateCacheVersion() is fast enough. Note that
> > Date.prototype.ToString() already does this
> > kind of invalidation in C++. See
> >
https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/date.cc&l=25
> > (ResetDateCache)
> > and
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/api.cc&l=6146
> 
> 
> Actually, there's a slightly better way. My CL (
> https://codereview.chromium.org/1973183002 ; it checks and update timezone
> everytime, though. My next step was to check DateCacheVersion) ) only updates
> the timezone instead of 
> invaldating the whole ICU DateTimeFormat object. It's a bit expensive to
create
> a new instance from the ground up. So, what my CL does is just
> update the timezone (nothing else is changed). 
> 
> We can land this CL and then follow this up by my optimization (with other
> comments addressed). 

A clarification: I expect little (if any) perf degradation for normal cases (NO
timezone change)
with this CL. My optimization ONLY affects when there is a timezone change,
which would be
rare.

[pgorszkowski, 2016-05-19 05:48:04]

The CQ bit was checked by pgorszkowski@opera.com

[pgorszkowski, 2016-05-19 05:48:04]

The CQ bit was unchecked by pgorszkowski@opera.com

[pgorszkowski, 2016-05-19 11:07:57]

https://codereview.chromium.org/1985423003/diff/40001/src/runtime/runtime-dat...
File src/runtime/runtime-date.cc (right):

https://codereview.chromium.org/1985423003/diff/40001/src/runtime/runtime-dat...
src/runtime/runtime-date.cc:55:
isolate->factory()->NewJSObject(isolate->array_function());
On 2016/05/18 17:39:57, Dan Ehrenberg wrote:
> Why is this wrapped in an array, rather than just returning the first element,
> if that is all that's used?

Currently it is kept as FixedArray, should we change it too? What kind of
'Object' should it be - any suggestions?

[Dan Ehrenberg, 2016-05-19 19:59:03]

lgtm

In conversation with jshin, sounds like the perf degradation would be
insignificant and the array follows patterns elsewhere.

[jungshik at Google, 2016-05-19 20:13:11]

LGTM 2 !

[jungshik at Google, 2016-05-19 20:13:18]

The CQ bit was checked by jshin@chromium.org

[commit-bot: I haz the power, 2016-05-19 20:13:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1985423003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1985423003/60001

[commit-bot: I haz the power, 2016-05-19 20:32:11]

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

[commit-bot: I haz the power, 2016-05-19 20:32:12]

Try jobs failed on following builders:
  v8_win_nosnap_shared_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)
  v8_win_nosnap_shared_rel_ng_triggered on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)

[pgorszkowski, 2016-05-19 20:34:59]

On 2016/05/19 20:32:12, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   v8_win_nosnap_shared_rel_ng on tryserver.v8 (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)
>   v8_win_nosnap_shared_rel_ng_triggered on tryserver.v8 (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)

I still need to fix unit tests.

[jungshik at Google, 2016-05-19 22:10:31]

Sorry that I missed one problem. Fortunately, trybot (Asan) caught it. See the
comment below.

https://codereview.chromium.org/1985423003/diff/60001/test/cctest/test-date.cc
File test/cctest/test-date.cc (right):

https://codereview.chromium.org/1985423003/diff/60001/test/cctest/test-date.c...
test/cctest/test-date.cc:170: TEST(DateCacheVersion) {
This test has to be enclosed by

#ifdef V8_I18N_SUPPORT
#endif 

because %DateCacheVersion is defined in runtime-i18n.cc which is not compiled
when V8_I18N_SUPPORT is not defined.

https://codereview.chromium.org/1985423003/diff/60001/test/cctest/test-date.c...
test/cctest/test-date.cc:178:
v8::Local<v8::Array>::Cast(CompileRun("%DateCacheVersion()"));
Asan test failed because DateCacheVersion is changed to return only a
version number instead of an array. 

This test has to be adjusted accordingly.

[jungshik at Google, 2016-05-19 22:13:26]

Description was changed from

==========
Invalidate defaultObjects if timezone changes

In case of calling 'toLocaleString', 'toLocaleTimeString' and
'toLocaleDateString' functions of 'Date' with empty 'locales' and
'options', DateTimeFormat is cached inside 'defaultObjects'.
If we change the timezone the cache is not invalidated.

BUG=v8:5022
TEST=see the bug
==========

to

==========
Invalidate defaultObjects if timezone changes

In case of calling 'toLocaleString', 'toLocaleTimeString' and
'toLocaleDateString' functions of 'Date' with empty 'locales' and
'options', DateTimeFormat is cached inside 'defaultObjects'.
If we change the timezone the cache is not invalidated.

BUG=v8:5022
TEST=cctest:DateCacheVersion. See the bug
CQ_INCLUDE_TRYBOTS=tryserver.v8:v8_linux_noi18n_rel_ng
==========

[jungshik at Google, 2016-05-19 23:11:58]

https://codereview.chromium.org/1985423003/diff/60001/src/js/i18n.js
File src/js/i18n.js (right):

https://codereview.chromium.org/1985423003/diff/60001/src/js/i18n.js#newcode2001
src/js/i18n.js:2001: var date_cache_version_holder = %DateCacheVersion();
nit: What's returned by DateCacheVersion is not a holder any more. 

How about |new_date_cache_version| instead of |date_cache_version_holder|?

[pgorszkowski, 2016-05-20 08:56:29]

The CQ bit was checked by pgorszkowski@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-05-20 08:56:35]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1985423003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1985423003/80001

[pgorszkowski, 2016-05-20 09:03:07]

The CQ bit was unchecked by pgorszkowski@opera.com

[pgorszkowski, 2016-05-20 13:14:00]

https://codereview.chromium.org/1985423003/diff/40001/src/runtime/runtime-dat...
File src/runtime/runtime-date.cc (right):

https://codereview.chromium.org/1985423003/diff/40001/src/runtime/runtime-dat...
src/runtime/runtime-date.cc:39: RUNTIME_FUNCTION(Runtime_DateCacheVersion) {
On 2016/05/18 17:39:57, Dan Ehrenberg wrote:
> If this is only used by Intl, better to put it in runtime-i18n.cc

Done.

[pgorszkowski, 2016-05-20 13:14:56]

https://codereview.chromium.org/1985423003/diff/60001/src/js/i18n.js
File src/js/i18n.js (right):

https://codereview.chromium.org/1985423003/diff/60001/src/js/i18n.js#newcode2001
src/js/i18n.js:2001: var date_cache_version_holder = %DateCacheVersion();
On 2016/05/19 23:11:58, jshin (jungshik at google) wrote:
> nit: What's returned by DateCacheVersion is not a holder any more. 
> 
> How about |new_date_cache_version| instead of |date_cache_version_holder|? 

Done.

https://codereview.chromium.org/1985423003/diff/60001/test/cctest/test-date.cc
File test/cctest/test-date.cc (right):

https://codereview.chromium.org/1985423003/diff/60001/test/cctest/test-date.c...
test/cctest/test-date.cc:170: TEST(DateCacheVersion) {
On 2016/05/19 22:10:31, jshin (jungshik at google) wrote:
> This test has to be enclosed by
> 
> #ifdef V8_I18N_SUPPORT
> #endif 
> 
> because %DateCacheVersion is defined in runtime-i18n.cc which is not compiled
> when V8_I18N_SUPPORT is not defined. 

Done.

https://codereview.chromium.org/1985423003/diff/60001/test/cctest/test-date.c...
test/cctest/test-date.cc:178:
v8::Local<v8::Array>::Cast(CompileRun("%DateCacheVersion()"));
On 2016/05/19 22:10:31, jshin (jungshik at google) wrote:
> Asan test failed because DateCacheVersion is changed to return only a
> version number instead of an array. 
> 
> This test has to be adjusted accordingly. 

Done.

[jungshik at Google, 2016-05-20 17:13:01]

The CQ bit was checked by jshin@chromium.org

[jungshik at Google, 2016-05-20 17:13:02]

The patchset sent to the CQ was uploaded after l-g-t-m from jshin@chromium.org,
littledan@chromium.org
Link to the patchset: https://codereview.chromium.org/1985423003/#ps80001
(title: "fixes after review")

[commit-bot: I haz the power, 2016-05-20 17:13:08]

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

[commit-bot: I haz the power, 2016-05-20 17:47:30]

Description was changed from

==========
Invalidate defaultObjects if timezone changes

In case of calling 'toLocaleString', 'toLocaleTimeString' and
'toLocaleDateString' functions of 'Date' with empty 'locales' and
'options', DateTimeFormat is cached inside 'defaultObjects'.
If we change the timezone the cache is not invalidated.

BUG=v8:5022
TEST=cctest:DateCacheVersion. See the bug
CQ_INCLUDE_TRYBOTS=tryserver.v8:v8_linux_noi18n_rel_ng
==========

to

==========
Invalidate defaultObjects if timezone changes

In case of calling 'toLocaleString', 'toLocaleTimeString' and
'toLocaleDateString' functions of 'Date' with empty 'locales' and
'options', DateTimeFormat is cached inside 'defaultObjects'.
If we change the timezone the cache is not invalidated.

BUG=v8:5022
TEST=cctest:DateCacheVersion. See the bug
CQ_INCLUDE_TRYBOTS=tryserver.v8:v8_linux_noi18n_rel_ng
==========

[commit-bot: I haz the power, 2016-05-20 17:47:31]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-05-20 17:47:54]

Description was changed from

==========
Invalidate defaultObjects if timezone changes

In case of calling 'toLocaleString', 'toLocaleTimeString' and
'toLocaleDateString' functions of 'Date' with empty 'locales' and
'options', DateTimeFormat is cached inside 'defaultObjects'.
If we change the timezone the cache is not invalidated.

BUG=v8:5022
TEST=cctest:DateCacheVersion. See the bug
CQ_INCLUDE_TRYBOTS=tryserver.v8:v8_linux_noi18n_rel_ng
==========

to

==========
Invalidate defaultObjects if timezone changes

In case of calling 'toLocaleString', 'toLocaleTimeString' and
'toLocaleDateString' functions of 'Date' with empty 'locales' and
'options', DateTimeFormat is cached inside 'defaultObjects'.
If we change the timezone the cache is not invalidated.

BUG=v8:5022
TEST=cctest:DateCacheVersion. See the bug
CQ_INCLUDE_TRYBOTS=tryserver.v8:v8_linux_noi18n_rel_ng

Committed: https://crrev.com/7afd712ae01cb40aa233ab80dad1b69a8b72a76c
Cr-Commit-Position: refs/heads/master@{#36420}
==========

[commit-bot: I haz the power, 2016-05-20 17:47:55]

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