Implement date library functions in C++.

Implement date library functions in C++.

Developed together with Andreas Rossberg based on:
  https://chromiumcodereview.appspot.com/9117034/
  https://chromiumcodereview.appspot.com/9307083/

R=rossberg@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=10983

[ulan, 2012-03-02 13:38:25]

Please take a look.

The first patch set contains your CLs 9117034 and 9307083.

Tests are coming later.

https://chromiumcodereview.appspot.com/9572008/diff/7001/src/date.cc
File src/date.cc (right):

https://chromiumcodereview.appspot.com/9572008/diff/7001/src/date.cc#newcode46
src/date.cc:46: static const int kYearsOffset = 400000;
The above constants are from runtime.cc

https://chromiumcodereview.appspot.com/9572008/diff/7001/src/date.cc#newcode77
src/date.cc:77: void DateCache::YearMonthDayFromDays(
Code in this function up to line 127 is from runtime.cc

https://chromiumcodereview.appspot.com/9572008/diff/7001/src/date.cc#newcode158
src/date.cc:158: int DateCache::DaysFromYearMonth(int year, int month) {
Code in this function is from runtime.cc

[rossberg, 2012-03-06 15:55:49]

Good stuff. Most comments are minor.

https://chromiumcodereview.appspot.com/9572008/diff/1/src/arm/lithium-codegen...
File src/arm/lithium-codegen-arm.cc (right):

https://chromiumcodereview.appspot.com/9572008/diff/1/src/arm/lithium-codegen...
src/arm/lithium-codegen-arm.cc:1446: #ifdef DEBUG
Any reason you removed the debug check in this instance?

https://chromiumcodereview.appspot.com/9572008/diff/1/src/ia32/lithium-codege...
File src/ia32/lithium-codegen-ia32.cc (right):

https://chromiumcodereview.appspot.com/9572008/diff/1/src/ia32/lithium-codege...
src/ia32/lithium-codegen-ia32.cc:1296: #ifdef DEBUG
This one got removed, too.

https://chromiumcodereview.appspot.com/9572008/diff/31/src/arm/full-codegen-a...
File src/arm/full-codegen-arm.cc (right):

https://chromiumcodereview.appspot.com/9572008/diff/31/src/arm/full-codegen-a...
src/arm/full-codegen-arm.cc:2940: Smi* index =
Smi::cast(*(args->at(1)->AsLiteral()->handle()));
Why did you change this? It requires more redundancy below.

https://chromiumcodereview.appspot.com/9572008/diff/31/src/arm/lithium-arm.h
File src/arm/lithium-arm.h (right):

https://chromiumcodereview.appspot.com/9572008/diff/31/src/arm/lithium-arm.h#...
src/arm/lithium-arm.h:995: LDateField(LOperand* date, LOperand* temp, Smi*
index) : index_(index) {
Again, I'm not fond of using Smi here. I don't see the advantage. It's a static
parameter, so it should be turned into a proper static type as early as
possible.

https://chromiumcodereview.appspot.com/9572008/diff/31/src/date.js
File src/date.js (right):

https://chromiumcodereview.appspot.com/9572008/diff/31/src/date.js#newcode247
src/date.js:247: function TimeStringUTC(date) {
Hm, this does three calls into the runtime. But I guess it doesn't matter much
for this function.

https://chromiumcodereview.appspot.com/9572008/diff/31/src/date.js#newcode518
src/date.js:518: var time = MakeTime(LOCAL_HOUR(this), LOCAL_MIN(this),
LOCAL_SEC(this), ms);
Not for this CL, but I wonder whether this computation (and similarly, the ones
in other setters) could be simplified to just saying

  date = LOCAL_DATE(this) - LOCAL_MS(this) + ms

Not sure if there are corner cases that might break, though.

https://chromiumcodereview.appspot.com/9572008/diff/31/src/hydrogen-instructi...
File src/hydrogen-instructions.h (right):

https://chromiumcodereview.appspot.com/9572008/diff/31/src/hydrogen-instructi...
src/hydrogen-instructions.h:4635: HDateField(HValue* date, Smi* index)
See above.

https://chromiumcodereview.appspot.com/9572008/diff/31/src/ia32/lithium-ia32.h
File src/ia32/lithium-ia32.h (right):

https://chromiumcodereview.appspot.com/9572008/diff/31/src/ia32/lithium-ia32....
src/ia32/lithium-ia32.h:1008: LDateField(LOperand* date, LOperand* temp, Smi*
index)
See above.

https://chromiumcodereview.appspot.com/9572008/diff/31/src/isolate.h
File src/isolate.h (right):

https://chromiumcodereview.appspot.com/9572008/diff/31/src/isolate.h#newcode1400
src/isolate.h:1400: 
Accidental newline?

https://chromiumcodereview.appspot.com/9572008/diff/31/src/objects-debug.cc
File src/objects-debug.cc (right):

https://chromiumcodereview.appspot.com/9572008/diff/31/src/objects-debug.cc#n...
src/objects-debug.cc:388: CHECK(weekday()->IsUndefined() || weekday()->IsSmi()
|| weekday()->IsNaN());
This is not checking the stamp field.

Also, you may want to reorder weekday to be consistent with object.h.

https://chromiumcodereview.appspot.com/9572008/diff/31/src/objects-inl.h
File src/objects-inl.h (right):

https://chromiumcodereview.appspot.com/9572008/diff/31/src/objects-inl.h#newc...
src/objects-inl.h:4137: ACCESSORS(JSDate, weekday, Object, kWeekdayOffset)
Nit: reorder for consistency.

https://chromiumcodereview.appspot.com/9572008/diff/31/src/objects.cc
File src/objects.cc (right):

https://chromiumcodereview.appspot.com/9572008/diff/31/src/objects.cc#newcode...
src/objects.cc:13207: ASSERT(index == kSecond);
I think in other places we tend to use a regular case, and then put
UNREACHABLE() after default.

https://chromiumcodereview.appspot.com/9572008/diff/31/src/objects.cc#newcode...
src/objects.cc:13223: if (index == kMillisecond) return
Smi::FromInt(time_in_day_ms % 1000);
Maybe consider using another 'switch' here for clarity.

https://chromiumcodereview.appspot.com/9572008/diff/31/src/objects.cc#newcode...
src/objects.cc:13224: if (index == kDays) return Smi::FromInt(days);
You could move this up before computing time_in_day_ms, because computing that
is redundant in this case.

https://chromiumcodereview.appspot.com/9572008/diff/31/src/objects.cc#newcode...
src/objects.cc:13238: int days = DateCache::DaysFromTime(time_ms);
You can move this after the kTimezoneOffset case.

https://chromiumcodereview.appspot.com/9572008/diff/31/src/objects.cc#newcode...
src/objects.cc:13250: if (index == kYearUTC) return Smi::FromInt(year);
Perhaps use a switch here.

https://chromiumcodereview.appspot.com/9572008/diff/31/src/objects.cc#newcode...
src/objects.cc:13264: ASSERT(index == kTimeInDayUTC);
See above, consider using an unreachable default.

https://chromiumcodereview.appspot.com/9572008/diff/31/src/objects.cc#newcode...
src/objects.cc:13292: int days, time_in_day_ms, year, month, day, weekday, hour,
min, sec;
Avoid uninitialized declarations if you can (like here).

https://chromiumcodereview.appspot.com/9572008/diff/31/src/objects.h
File src/objects.h (right):

https://chromiumcodereview.appspot.com/9572008/diff/31/src/objects.h#newcode6040
src/objects.h:6040: static MaybeObject* GetField(Object* date, Smi* index);
Why is this a static method?
And why not give index type FieldIndex?

https://chromiumcodereview.appspot.com/9572008/diff/31/src/objects.h#newcode6042
src/objects.h:6042: void SetValue(Object* value, bool is_value_nan);
I would remove the redundant flag parameter and just test value.

https://chromiumcodereview.appspot.com/9572008/diff/31/src/runtime.cc
File src/runtime.cc (right):

https://chromiumcodereview.appspot.com/9572008/diff/31/src/runtime.cc#newcode...
src/runtime.cc:7545: is_value_nan = true;
Indentation.

https://chromiumcodereview.appspot.com/9572008/diff/31/src/runtime.cc#newcode...
src/runtime.cc:7548: isolate->heap()->AllocateHeapNumber(DoubleToInteger(time));
If is_utc is true, this allocation seems redundant, since you could just reuse
the argument.

https://chromiumcodereview.appspot.com/9572008/diff/31/src/runtime.cc#newcode...
src/runtime.cc:7552: date->SetValue(value, is_value_nan);
Why do you need the extra flag? Can't the function just check the value?

https://chromiumcodereview.appspot.com/9572008/diff/31/src/runtime.cc#newcode...
src/runtime.cc:9030: const char* zone =
OS::LocalTimezone(static_cast<double>(time));
Do these conversions (here and below) actually require an explicit cast?

https://chromiumcodereview.appspot.com/9572008/diff/31/src/x64/full-codegen-x...
File src/x64/full-codegen-x64.cc (right):

https://chromiumcodereview.appspot.com/9572008/diff/31/src/x64/full-codegen-x...
src/x64/full-codegen-x64.cc:2852: kPointerSize * index->value()));
Nit: indentation.

https://chromiumcodereview.appspot.com/9572008/diff/10011/src/date.cc
File src/date.cc (right):

https://chromiumcodereview.appspot.com/9572008/diff/10011/src/date.cc#newcode46
src/date.cc:46: static const int kYearsOffset = 400000;
Not your change, but I don't understand the last two constants, or how they are
used below. In case you do, could you add some comments?

https://chromiumcodereview.appspot.com/9572008/diff/10011/src/date.cc#newcode58
src/date.cc:58: for (int i = 0; i < kDSTSize; i++) {
Nit: Google style guide wants ++i.

https://chromiumcodereview.appspot.com/9572008/diff/10011/src/date.cc#newcode257
src/date.cc:257: // This swap helps the optimistic fast check in subsequent
invokations.
Typo: invocations.

https://chromiumcodereview.appspot.com/9572008/diff/10011/src/date.cc#newcode272
src/date.cc:272: ExtendTheAfterSegment(new_after_start_sec, offset_ms);
IIUC, then at this point, you already know the result offset_ms, but if
before_->offset_ms is different from it, you will skip the next conditional and
still do a binary search below.

https://chromiumcodereview.appspot.com/9572008/diff/10011/src/date.cc#newcode286
src/date.cc:286: // but limit it to four iterations.
Hm, I don't understand, why only 4? Don't you need 5 to cover 19 days? Also,
this should be a constant, defined next to kDefaultDSTDeltaInSec.

In fact, why do you need to limit it at all if you know that it always succeeds?

https://chromiumcodereview.appspot.com/9572008/diff/10011/src/date.cc#newcode287
src/date.cc:287: for (int i = 4; i >= 0; i--) {
Nit: --i

https://chromiumcodereview.appspot.com/9572008/diff/10011/src/date.cc#newcode323
src/date.cc:323: }
Can't you return here directly? Both before_ and after_ point to invalid
segments at this point.

https://chromiumcodereview.appspot.com/9572008/diff/10011/src/date.cc#newcode326
src/date.cc:326: for (int i = 0; i < kDSTSize; i++) {
Nit: ++i again.

https://chromiumcodereview.appspot.com/9572008/diff/10011/src/date.cc#newcode354
src/date.cc:354: after->start_sec > before->end_sec);
Nit: Maybe turn around the comparison.

https://chromiumcodereview.appspot.com/9572008/diff/10011/src/date.cc#newcode356
src/date.cc:356: before->last_used = ++dst_usage_counter_;
Is it intentional that you update the usage counter even for invalid segments?

https://chromiumcodereview.appspot.com/9572008/diff/10011/src/date.h
File src/date.h (right):

https://chromiumcodereview.appspot.com/9572008/diff/10011/src/date.h#newcode55
src/date.h:55: // Assume that no local offset exceeds ten days.
Why not just use MAX_INT?

https://chromiumcodereview.appspot.com/9572008/diff/10011/src/date.h#newcode62
src/date.h:62: DateCache() : stamp_(0) {
If stamp_ is a Smi, this should better be Smi::FromInt(0).

https://chromiumcodereview.appspot.com/9572008/diff/10011/src/date.h#newcode64
src/date.h:64: counter_ = 0;
You could put this in the initializer list. (Or remove it, see below.)

https://chromiumcodereview.appspot.com/9572008/diff/10011/src/date.h#newcode112
src/date.h:112: time_ms = EquivalentTime(time_ms);
Nit: wrong indentation.

https://chromiumcodereview.appspot.com/9572008/diff/10011/src/date.h#newcode186
src/date.h:186: // printf("counter %d\n", counter_);
Leftover?

https://chromiumcodereview.appspot.com/9572008/diff/10011/src/date.h#newcode223
src/date.h:223: // Both segments might be invalid.
Add that last_used counters get updated by this function (wasn't obvious to me,
and confused me for a while).

https://chromiumcodereview.appspot.com/9572008/diff/10011/src/date.h#newcode257
src/date.h:257: int counter_;
What is counter_? Was that just a debugging device? If so, please remove it
before committing.

https://chromiumcodereview.appspot.com/9572008/diff/10011/src/isolate.cc
File src/isolate.cc (right):

https://chromiumcodereview.appspot.com/9572008/diff/10011/src/isolate.cc#newc...
src/isolate.cc:1627: 
Accidental newline?

https://chromiumcodereview.appspot.com/9572008/diff/10011/test/cctest/test-da...
File test/cctest/test-date.cc (right):

https://chromiumcodereview.appspot.com/9572008/diff/10011/test/cctest/test-da...
test/cctest/test-date.cc:131: new DateCacheMock(-36000000, rules, sizeof(rules)
/ sizeof(rules[0]));
What is that constant?
Also, you can use the ARRAY_SIZE macro for the last argument.

https://chromiumcodereview.appspot.com/9572008/diff/10011/test/cctest/test-da...
test/cctest/test-date.cc:132: isolate->set_date_cache(date_cache);
Nit: I wouldn't mind a newline here.

[ulan, 2012-03-07 10:55:21]

Thank for reviewing this. I uploaded a new patch set.

http://codereview.chromium.org/9572008/diff/31/src/arm/full-codegen-arm.cc
File src/arm/full-codegen-arm.cc (right):

http://codereview.chromium.org/9572008/diff/31/src/arm/full-codegen-arm.cc#ne...
src/arm/full-codegen-arm.cc:2940: Smi* index =
Smi::cast(*(args->at(1)->AsLiteral()->handle()));
On 2012/03/06 15:55:50, rossberg wrote:
> Why did you change this? It requires more redundancy below.
In other platforms, index is pushed onto stack before calling to runtime. So I
thought it's better to keep it as Smi. In ARM it is Smi for consistency.

http://codereview.chromium.org/9572008/diff/31/src/arm/lithium-arm.h
File src/arm/lithium-arm.h (right):

http://codereview.chromium.org/9572008/diff/31/src/arm/lithium-arm.h#newcode995
src/arm/lithium-arm.h:995: LDateField(LOperand* date, LOperand* temp, Smi*
index) : index_(index) {
On 2012/03/06 15:55:50, rossberg wrote:
> Again, I'm not fond of using Smi here. I don't see the advantage. It's a
static
> parameter, so it should be turned into a proper static type as early as
> possible.
Replied above (in full-codegen-arm.cc).

http://codereview.chromium.org/9572008/diff/31/src/date.js
File src/date.js (right):

http://codereview.chromium.org/9572008/diff/31/src/date.js#newcode247
src/date.js:247: function TimeStringUTC(date) {
On 2012/03/06 15:55:50, rossberg wrote:
> Hm, this does three calls into the runtime. But I guess it doesn't matter much
> for this function.
An alternative would be to get the time in day and then use division and modulo
to get hour, min, sec. I am not sure which one is faster. Leaving this as TODO
for another CL.

http://codereview.chromium.org/9572008/diff/31/src/date.js#newcode518
src/date.js:518: var time = MakeTime(LOCAL_HOUR(this), LOCAL_MIN(this),
LOCAL_SEC(this), ms);
On 2012/03/06 15:55:50, rossberg wrote:
> Not for this CL, but I wonder whether this computation (and similarly, the
ones
> in other setters) could be simplified to just saying
> 
>   date = LOCAL_DATE(this) - LOCAL_MS(this) + ms
> 
> Not sure if there are corner cases that might break, though.
Agreed, leaving this for another CL.

http://codereview.chromium.org/9572008/diff/31/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

http://codereview.chromium.org/9572008/diff/31/src/hydrogen-instructions.h#ne...
src/hydrogen-instructions.h:4635: HDateField(HValue* date, Smi* index)
On 2012/03/06 15:55:50, rossberg wrote:
> See above.

Done.

http://codereview.chromium.org/9572008/diff/31/src/ia32/lithium-ia32.h
File src/ia32/lithium-ia32.h (right):

http://codereview.chromium.org/9572008/diff/31/src/ia32/lithium-ia32.h#newcod...
src/ia32/lithium-ia32.h:1008: LDateField(LOperand* date, LOperand* temp, Smi*
index)
On 2012/03/06 15:55:50, rossberg wrote:
> See above.

Done.

http://codereview.chromium.org/9572008/diff/31/src/isolate.h
File src/isolate.h (right):

http://codereview.chromium.org/9572008/diff/31/src/isolate.h#newcode1400
src/isolate.h:1400: 
On 2012/03/06 15:55:50, rossberg wrote:
> Accidental newline?

Done.

http://codereview.chromium.org/9572008/diff/31/src/objects-debug.cc
File src/objects-debug.cc (right):

http://codereview.chromium.org/9572008/diff/31/src/objects-debug.cc#newcode388
src/objects-debug.cc:388: CHECK(weekday()->IsUndefined() || weekday()->IsSmi()
|| weekday()->IsNaN());
On 2012/03/06 15:55:50, rossberg wrote:
> This is not checking the stamp field.
> 
> Also, you may want to reorder weekday to be consistent with object.h.

Done.

http://codereview.chromium.org/9572008/diff/31/src/objects-inl.h
File src/objects-inl.h (right):

http://codereview.chromium.org/9572008/diff/31/src/objects-inl.h#newcode4137
src/objects-inl.h:4137: ACCESSORS(JSDate, weekday, Object, kWeekdayOffset)
On 2012/03/06 15:55:50, rossberg wrote:
> Nit: reorder for consistency.

Done.

http://codereview.chromium.org/9572008/diff/31/src/objects.cc
File src/objects.cc (right):

http://codereview.chromium.org/9572008/diff/31/src/objects.cc#newcode13207
src/objects.cc:13207: ASSERT(index == kSecond);
On 2012/03/06 15:55:50, rossberg wrote:
> I think in other places we tend to use a regular case, and then put
> UNREACHABLE() after default.

Done.

http://codereview.chromium.org/9572008/diff/31/src/objects.cc#newcode13223
src/objects.cc:13223: if (index == kMillisecond) return
Smi::FromInt(time_in_day_ms % 1000);
On 2012/03/06 15:55:50, rossberg wrote:
> Maybe consider using another 'switch' here for clarity.
Moved the "days" case up, now only two cases are left here.

http://codereview.chromium.org/9572008/diff/31/src/objects.cc#newcode13224
src/objects.cc:13224: if (index == kDays) return Smi::FromInt(days);
On 2012/03/06 15:55:50, rossberg wrote:
> You could move this up before computing time_in_day_ms, because computing that
> is redundant in this case.

Done.

http://codereview.chromium.org/9572008/diff/31/src/objects.cc#newcode13238
src/objects.cc:13238: int days = DateCache::DaysFromTime(time_ms);
On 2012/03/06 15:55:50, rossberg wrote:
> You can move this after the kTimezoneOffset case.

Done.

http://codereview.chromium.org/9572008/diff/31/src/objects.cc#newcode13250
src/objects.cc:13250: if (index == kYearUTC) return Smi::FromInt(year);
On 2012/03/06 15:55:50, rossberg wrote:
> Perhaps use a switch here.
I am not sure, just three cases.

http://codereview.chromium.org/9572008/diff/31/src/objects.cc#newcode13264
src/objects.cc:13264: ASSERT(index == kTimeInDayUTC);
On 2012/03/06 15:55:50, rossberg wrote:
> See above, consider using an unreachable default.

Done.

http://codereview.chromium.org/9572008/diff/31/src/objects.cc#newcode13292
src/objects.cc:13292: int days, time_in_day_ms, year, month, day, weekday, hour,
min, sec;
On 2012/03/06 15:55:50, rossberg wrote:
> Avoid uninitialized declarations if you can (like here).

Done.

http://codereview.chromium.org/9572008/diff/31/src/objects.h
File src/objects.h (right):

http://codereview.chromium.org/9572008/diff/31/src/objects.h#newcode6040
src/objects.h:6040: static MaybeObject* GetField(Object* date, Smi* index);
On 2012/03/06 15:55:50, rossberg wrote:
> Why is this a static method?
> And why not give index type FieldIndex?
It is called from generated code and the index is Smi because it is passed via
stack in ia32.

http://codereview.chromium.org/9572008/diff/31/src/objects.h#newcode6042
src/objects.h:6042: void SetValue(Object* value, bool is_value_nan);
On 2012/03/06 15:55:50, rossberg wrote:
> I would remove the redundant flag parameter and just test value.
This would cost us redundant operations.

http://codereview.chromium.org/9572008/diff/31/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/9572008/diff/31/src/runtime.cc#newcode7545
src/runtime.cc:7545: is_value_nan = true;
On 2012/03/06 15:55:50, rossberg wrote:
> Indentation.

Done.

http://codereview.chromium.org/9572008/diff/31/src/runtime.cc#newcode7548
src/runtime.cc:7548: isolate->heap()->AllocateHeapNumber(DoubleToInteger(time));
On 2012/03/06 15:55:50, rossberg wrote:
> If is_utc is true, this allocation seems redundant, since you could just reuse
> the argument.
Unfortunately we need to truncate the time (DoubleToInteger).

http://codereview.chromium.org/9572008/diff/31/src/runtime.cc#newcode7552
src/runtime.cc:7552: date->SetValue(value, is_value_nan);
On 2012/03/06 15:55:50, rossberg wrote:
> Why do you need the extra flag? Can't the function just check the value?
Saving one isnan computation.

http://codereview.chromium.org/9572008/diff/31/src/runtime.cc#newcode9030
src/runtime.cc:9030: const char* zone =
OS::LocalTimezone(static_cast<double>(time));
On 2012/03/06 15:55:50, rossberg wrote:
> Do these conversions (here and below) actually require an explicit cast?

Done.

http://codereview.chromium.org/9572008/diff/31/src/x64/full-codegen-x64.cc
File src/x64/full-codegen-x64.cc (right):

http://codereview.chromium.org/9572008/diff/31/src/x64/full-codegen-x64.cc#ne...
src/x64/full-codegen-x64.cc:2852: kPointerSize * index->value()));
On 2012/03/06 15:55:50, rossberg wrote:
> Nit: indentation.

Done.

http://codereview.chromium.org/9572008/diff/10011/src/date.cc
File src/date.cc (right):

http://codereview.chromium.org/9572008/diff/10011/src/date.cc#newcode46
src/date.cc:46: static const int kYearsOffset = 400000;
On 2012/03/06 15:55:50, rossberg wrote:
> Not your change, but I don't understand the last two constants, or how they
are
> used below. In case you do, could you add some comments?
It's complicated :)
See comment in DaysFromYearMonth.
I should probably clean this up in another CL.

http://codereview.chromium.org/9572008/diff/10011/src/date.cc#newcode58
src/date.cc:58: for (int i = 0; i < kDSTSize; i++) {
On 2012/03/06 15:55:50, rossberg wrote:
> Nit: Google style guide wants ++i.

Done.

http://codereview.chromium.org/9572008/diff/10011/src/date.cc#newcode257
src/date.cc:257: // This swap helps the optimistic fast check in subsequent
invokations.
On 2012/03/06 15:55:50, rossberg wrote:
> Typo: invocations.

Done.

http://codereview.chromium.org/9572008/diff/10011/src/date.cc#newcode272
src/date.cc:272: ExtendTheAfterSegment(new_after_start_sec, offset_ms);
On 2012/03/06 15:55:50, rossberg wrote:
> IIUC, then at this point, you already know the result offset_ms, but if
> before_->offset_ms is different from it, you will skip the next conditional
and
> still do a binary search below.
If the next condition holds then the result offset must be equal to
before_->offset_ms, because the time_sec is between before_->end_sec and
after->start_sec and that range is about 19 days so DST cannot change twice
there.

http://codereview.chromium.org/9572008/diff/10011/src/date.cc#newcode286
src/date.cc:286: // but limit it to four iterations.
On 2012/03/06 15:55:50, rossberg wrote:
> Hm, I don't understand, why only 4? Don't you need 5 to cover 19 days? Also,
> this should be a constant, defined next to kDefaultDSTDeltaInSec.
> 
> In fact, why do you need to limit it at all if you know that it always
succeeds?
I updated the comment. We need about 30 = log(19 * kMsPerDay) iterations and
that is too expensive, and the exact change point won't be probably needed
anyway.
Four iterations narrow the range to about one day, which seems to be good
enough.

http://codereview.chromium.org/9572008/diff/10011/src/date.cc#newcode287
src/date.cc:287: for (int i = 4; i >= 0; i--) {
On 2012/03/06 15:55:50, rossberg wrote:
> Nit: --i

Done.

http://codereview.chromium.org/9572008/diff/10011/src/date.cc#newcode323
src/date.cc:323: }
On 2012/03/06 15:55:50, rossberg wrote:
> Can't you return here directly? Both before_ and after_ point to invalid
> segments at this point.
Yes, but I would prefer to have one exit from this function.

http://codereview.chromium.org/9572008/diff/10011/src/date.cc#newcode326
src/date.cc:326: for (int i = 0; i < kDSTSize; i++) {
On 2012/03/06 15:55:50, rossberg wrote:
> Nit: ++i again.

Done.

http://codereview.chromium.org/9572008/diff/10011/src/date.cc#newcode354
src/date.cc:354: after->start_sec > before->end_sec);
On 2012/03/06 15:55:50, rossberg wrote:
> Nit: Maybe turn around the comparison.

Done.

http://codereview.chromium.org/9572008/diff/10011/src/date.cc#newcode356
src/date.cc:356: before->last_used = ++dst_usage_counter_;
On 2012/03/06 15:55:50, rossberg wrote:
> Is it intentional that you update the usage counter even for invalid segments?
Yes, it simplifies the code of DaylightSavingsOffsetInMs().
Added a comment in the code.

http://codereview.chromium.org/9572008/diff/10011/src/date.h
File src/date.h (right):

http://codereview.chromium.org/9572008/diff/10011/src/date.h#newcode55
src/date.h:55: // Assume that no local offset exceeds ten days.
On 2012/03/06 15:55:50, rossberg wrote:
> Why not just use MAX_INT?

Done.

http://codereview.chromium.org/9572008/diff/10011/src/date.h#newcode62
src/date.h:62: DateCache() : stamp_(0) {
On 2012/03/06 15:55:50, rossberg wrote:
> If stamp_ is a Smi, this should better be Smi::FromInt(0).
This would require including "objects-inl.h", is it OK?

http://codereview.chromium.org/9572008/diff/10011/src/date.h#newcode64
src/date.h:64: counter_ = 0;
On 2012/03/06 15:55:50, rossberg wrote:
> You could put this in the initializer list. (Or remove it, see below.)

Done.

http://codereview.chromium.org/9572008/diff/10011/src/date.h#newcode112
src/date.h:112: time_ms = EquivalentTime(time_ms);
On 2012/03/06 15:55:50, rossberg wrote:
> Nit: wrong indentation.

Done.

http://codereview.chromium.org/9572008/diff/10011/src/date.h#newcode186
src/date.h:186: // printf("counter %d\n", counter_);
On 2012/03/06 15:55:50, rossberg wrote:
> Leftover?
Yep. Done.

http://codereview.chromium.org/9572008/diff/10011/src/date.h#newcode223
src/date.h:223: // Both segments might be invalid.
On 2012/03/06 15:55:50, rossberg wrote:
> Add that last_used counters get updated by this function (wasn't obvious to
me,
> and confused me for a while).

Done.

http://codereview.chromium.org/9572008/diff/10011/src/date.h#newcode257
src/date.h:257: int counter_;
On 2012/03/06 15:55:50, rossberg wrote:
> What is counter_? Was that just a debugging device? If so, please remove it
> before committing.

Done.

http://codereview.chromium.org/9572008/diff/10011/src/isolate.cc
File src/isolate.cc (right):

http://codereview.chromium.org/9572008/diff/10011/src/isolate.cc#newcode1627
src/isolate.cc:1627: 
On 2012/03/06 15:55:50, rossberg wrote:
> Accidental newline?

Done.

http://codereview.chromium.org/9572008/diff/10011/test/cctest/test-date.cc
File test/cctest/test-date.cc (right):

http://codereview.chromium.org/9572008/diff/10011/test/cctest/test-date.cc#ne...
test/cctest/test-date.cc:131: new DateCacheMock(-36000000, rules, sizeof(rules)
/ sizeof(rules[0]));
On 2012/03/06 15:55:50, rossberg wrote:
> What is that constant?
> Also, you can use the ARRAY_SIZE macro for the last argument.

Done.

http://codereview.chromium.org/9572008/diff/10011/test/cctest/test-date.cc#ne...
test/cctest/test-date.cc:132: isolate->set_date_cache(date_cache);
On 2012/03/06 15:55:50, rossberg wrote:
> Nit: I wouldn't mind a newline here.

Done.

[rossberg, 2012-03-07 13:48:38]

https://chromiumcodereview.appspot.com/9572008/diff/10011/src/date.cc
File src/date.cc (right):

https://chromiumcodereview.appspot.com/9572008/diff/10011/src/date.cc#newcode272
src/date.cc:272: ExtendTheAfterSegment(new_after_start_sec, offset_ms);
On 2012/03/07 10:55:21, ulan wrote:
> On 2012/03/06 15:55:50, rossberg wrote:
> > IIUC, then at this point, you already know the result offset_ms, but if
> > before_->offset_ms is different from it, you will skip the next conditional
> and
> > still do a binary search below.
> If the next condition holds then the result offset must be equal to
> before_->offset_ms, because the time_sec is between before_->end_sec and
> after->start_sec and that range is about 19 days so DST cannot change twice
> there.

Yes, but my question was about the case where the next condition does _not_
hold. It seems like the code is starting a redundant binary search in that
situation.

https://chromiumcodereview.appspot.com/9572008/diff/10011/src/date.cc#newcode286
src/date.cc:286: // but limit it to four iterations.
On 2012/03/07 10:55:21, ulan wrote:
> On 2012/03/06 15:55:50, rossberg wrote:
> > Hm, I don't understand, why only 4? Don't you need 5 to cover 19 days? Also,
> > this should be a constant, defined next to kDefaultDSTDeltaInSec.
> > 
> > In fact, why do you need to limit it at all if you know that it always
> succeeds?
> I updated the comment. We need about 30 = log(19 * kMsPerDay) iterations and
> that is too expensive, and the exact change point won't be probably needed
> anyway.
> Four iterations narrow the range to about one day, which seems to be good
> enough.

I don't understand. Don't you run into UNREACHABLE below if the 4 iterations
don't suffice?

https://chromiumcodereview.appspot.com/9572008/diff/10011/src/date.cc#newcode356
src/date.cc:356: before->last_used = ++dst_usage_counter_;
On 2012/03/07 10:55:21, ulan wrote:
> On 2012/03/06 15:55:50, rossberg wrote:
> > Is it intentional that you update the usage counter even for invalid
segments?
> Yes, it simplifies the code of DaylightSavingsOffsetInMs().
> Added a comment in the code.

How? I would have expected that you never count invalid segments as "used" at
all.

https://chromiumcodereview.appspot.com/9572008/diff/10011/src/date.h
File src/date.h (right):

https://chromiumcodereview.appspot.com/9572008/diff/10011/src/date.h#newcode62
src/date.h:62: DateCache() : stamp_(0) {
On 2012/03/07 10:55:21, ulan wrote:
> On 2012/03/06 15:55:50, rossberg wrote:
> > If stamp_ is a Smi, this should better be Smi::FromInt(0).
> This would require including "objects-inl.h", is it OK?

If that's how it has to be, then that's how it has to be. :)

https://chromiumcodereview.appspot.com/9572008/diff/21010/src/date.h
File src/date.h (right):

https://chromiumcodereview.appspot.com/9572008/diff/21010/src/date.h#newcode122
src/date.h:122: return  time_ms + LocalOffsetInMs() +
DaylightSavingsOffsetInMs(time_ms);
Nit: double space.

[ulan, 2012-03-07 14:39:19]

Please take another look.

https://chromiumcodereview.appspot.com/9572008/diff/10011/src/date.cc
File src/date.cc (right):

https://chromiumcodereview.appspot.com/9572008/diff/10011/src/date.cc#newcode272
src/date.cc:272: ExtendTheAfterSegment(new_after_start_sec, offset_ms);
On 2012/03/07 13:48:38, rossberg wrote:
> On 2012/03/07 10:55:21, ulan wrote:
> > On 2012/03/06 15:55:50, rossberg wrote:
> > > IIUC, then at this point, you already know the result offset_ms, but if
> > > before_->offset_ms is different from it, you will skip the next
conditional
> > and
> > > still do a binary search below.
> > If the next condition holds then the result offset must be equal to
> > before_->offset_ms, because the time_sec is between before_->end_sec and
> > after->start_sec and that range is about 19 days so DST cannot change twice
> > there.
> 
> Yes, but my question was about the case where the next condition does _not_
> hold. It seems like the code is starting a redundant binary search in that
> situation.
As discussed offline, the binary search is not redundant because offset_ms is
computed for new_after_start_sec.
I renamed the offset_ms to avoid confusion.

https://chromiumcodereview.appspot.com/9572008/diff/10011/src/date.cc#newcode286
src/date.cc:286: // but limit it to four iterations.
On 2012/03/07 13:48:38, rossberg wrote:
> On 2012/03/07 10:55:21, ulan wrote:
> > On 2012/03/06 15:55:50, rossberg wrote:
> > > Hm, I don't understand, why only 4? Don't you need 5 to cover 19 days?
Also,
> > > this should be a constant, defined next to kDefaultDSTDeltaInSec.
> > > 
> > > In fact, why do you need to limit it at all if you know that it always
> > succeeds?
> > I updated the comment. We need about 30 = log(19 * kMsPerDay) iterations and
> > that is too expensive, and the exact change point won't be probably needed
> > anyway.
> > Four iterations narrow the range to about one day, which seems to be good
> > enough.
> 
> I don't understand. Don't you run into UNREACHABLE below if the 4 iterations
> don't suffice?
As discussed offline, the last iteration computes the offset for time_sec.

https://chromiumcodereview.appspot.com/9572008/diff/10011/src/date.cc#newcode356
src/date.cc:356: before->last_used = ++dst_usage_counter_;
On 2012/03/07 13:48:38, rossberg wrote:
> On 2012/03/07 10:55:21, ulan wrote:
> > On 2012/03/06 15:55:50, rossberg wrote:
> > > Is it intentional that you update the usage counter even for invalid
> segments?
> > Yes, it simplifies the code of DaylightSavingsOffsetInMs().
> > Added a comment in the code.
> 
> How? I would have expected that you never count invalid segments as "used" at
> all.
Moved last usage update into the caller function.

https://chromiumcodereview.appspot.com/9572008/diff/10011/src/date.h
File src/date.h (right):

https://chromiumcodereview.appspot.com/9572008/diff/10011/src/date.h#newcode62
src/date.h:62: DateCache() : stamp_(0) {
On 2012/03/07 13:48:38, rossberg wrote:
> On 2012/03/07 10:55:21, ulan wrote:
> > On 2012/03/06 15:55:50, rossberg wrote:
> > > If stamp_ is a Smi, this should better be Smi::FromInt(0).
> > This would require including "objects-inl.h", is it OK?
> 
> If that's how it has to be, then that's how it has to be. :)
Tried, but it introduces circular includes.

https://chromiumcodereview.appspot.com/9572008/diff/21010/src/date.h
File src/date.h (right):

https://chromiumcodereview.appspot.com/9572008/diff/21010/src/date.h#newcode122
src/date.h:122: return  time_ms + LocalOffsetInMs() +
DaylightSavingsOffsetInMs(time_ms);
On 2012/03/07 13:48:38, rossberg wrote:
> Nit: double space.

Done.

[rossberg, 2012-03-07 15:21:25]

lgtm