Issue 9117034: New class for Date objects: caches individual date components.

Issue 9117034: New class for Date objects: caches individual date components. (Closed)

Created:
8 years, 11 months ago by rossberg

Modified:
8 years, 9 months ago

Reviewers:
ulan

CC:
v8-dev

Base URL:
https://v8.googlecode.com/svn/branches/bleeding_edge

Visibility:
Public.

More Reviews

Description

New class for Date objects: caches individual date components. First step, cache slots not used yet. R=ulan@chromium.org BUG= TEST= Committed: https://code.google.com/p/v8/source/detail?r=10981

Patch Set 2 : Added ARM and x64 ports. #

Patch Set 3 : Removed type checks and write barriers; added Hydrogen instr for writing Date fields. #

Patch Set 4 : Add constant for index of first barrier-free slot. #

Created: 8 years, 11 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+699 lines, -32 lines)			Patch
M	include/v8.h	View		1 chunk	+1 line, -1 line	0 comments	Download
M	src/api.cc	View	1 2	1 chunk	+2 lines, -2 lines	0 comments	Download
M	src/arm/full-codegen-arm.cc	View	1 2 3	2 chunks	+50 lines, -0 lines	0 comments	Download
M	src/arm/lithium-arm.h	View	1 2	2 chunks	+38 lines, -1 line	0 comments	Download
M	src/arm/lithium-arm.cc	View	1 2	1 chunk	+16 lines, -0 lines	0 comments	Download
M	src/arm/lithium-codegen-arm.cc	View	1 2 3	1 chunk	+43 lines, -0 lines	0 comments	Download
M	src/bootstrapper.cc	View		1 chunk	+1 line, -1 line	0 comments	Download
M	src/date.js	View	1 2	18 chunks	+19 lines, -19 lines	0 comments	Download
M	src/hydrogen.cc	View	1 2	2 chunks	+28 lines, -0 lines	0 comments	Download
M	src/hydrogen-instructions.h	View	1 2	2 chunks	+42 lines, -1 line	0 comments	Download
M	src/ia32/full-codegen-ia32.cc	View	1 2 3	2 chunks	+49 lines, -0 lines	0 comments	Download
M	src/ia32/lithium-codegen-ia32.cc	View	1 2 3	2 chunks	+44 lines, -0 lines	0 comments	Download
M	src/ia32/lithium-ia32.h	View	1 2	2 chunks	+39 lines, -1 line	0 comments	Download
M	src/ia32/lithium-ia32.cc	View	1 2	1 chunk	+17 lines, -0 lines	0 comments	Download
M	src/macros.py	View	1 2 3	1 chunk	+16 lines, -1 line	0 comments	Download
M	src/objects.h	View	1 2 3	6 chunks	+59 lines, -1 line	0 comments	Download
M	src/objects.cc	View		2 chunks	+2 lines, -1 line	0 comments	Download
M	src/objects-debug.cc	View		2 chunks	+42 lines, -0 lines	0 comments	Download
M	src/objects-inl.h	View		3 chunks	+20 lines, -0 lines	0 comments	Download
M	src/objects-printer.cc	View	1 2	2 chunks	+23 lines, -0 lines	0 comments	Download
M	src/objects-visiting.cc	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	src/runtime.h	View		1 chunk	+2 lines, -0 lines	0 comments	Download
M	src/x64/full-codegen-x64.cc	View	1 2 3	2 chunks	+49 lines, -0 lines	0 comments	Download
M	src/x64/lithium-codegen-x64.cc	View	1 2 3	1 chunk	+41 lines, -0 lines	0 comments	Download
M	src/x64/lithium-x64.h	View	1 2	2 chunks	+36 lines, -1 line	0 comments	Download
M	src/x64/lithium-x64.cc	View	1 2	1 chunk	+15 lines, -0 lines	0 comments	Download
M	test/mjsunit/fuzz-natives.js	View		1 chunk	+4 lines, -2 lines	0 comments	Download

Messages

Total messages: 5 (0 generated)

Expand Messages | Collapse Messages

ulan

First round of comments. https://chromiumcodereview.appspot.com/9117034/diff/1/src/hydrogen-instructions.h File src/hydrogen-instructions.h (right): https://chromiumcodereview.appspot.com/9117034/diff/1/src/hydrogen-instructions.h#newcode183 src/hydrogen-instructions.h:183: V(DateField) Don't forget SetDateField. https://chromiumcodereview.appspot.com/9117034/diff/1/src/hydrogen-instructions.h#newcode4514 ...

8 years, 11 months ago (2012-01-25 13:08:48 UTC) #3

rossberg

I removed all type checks in generated code, except in debug mode. Removed write barriers ...

8 years, 11 months ago (2012-01-25 15:48:37 UTC) #4

I removed all type checks in generated code, except in debug mode. Removed write
barriers except for the value slot. Also added SetDateField instruction to
Hydrogen instead of going through the more expensive generic StoreNamed.

https://chromiumcodereview.appspot.com/9117034/diff/1/src/hydrogen-instructio...
File src/hydrogen-instructions.h (right):

https://chromiumcodereview.appspot.com/9117034/diff/1/src/hydrogen-instructio...
src/hydrogen-instructions.h:183: V(DateField)
On 2012/01/25 13:08:48, ulan wrote:
> Don't forget SetDateField.

There was no HSetDateField instruction. ;) However, it exists now.

https://chromiumcodereview.appspot.com/9117034/diff/1/src/hydrogen-instructio...
src/hydrogen-instructions.h:4514: class HDateField: public HUnaryOperation {
On 2012/01/25 13:08:48, ulan wrote:
> This instruction has two arguments, so HBinaryOperation or
> HTemplateInstruction<2> seem more appropriate.

No, it's a unary instruction. The index is merely a static attribute.

https://chromiumcodereview.appspot.com/9117034/diff/1/src/hydrogen-instructio...
src/hydrogen-instructions.h:4518: set_representation(Representation::Tagged());
On 2012/01/25 13:08:48, ulan wrote:
> Is it possible to use Int32 representation and deoptimize if NaN?

Then I would need to implement the untagging in the instruction. I don't see
what the win would be. The integers cannot be untagged on the object itself.

https://chromiumcodereview.appspot.com/9117034/diff/1/src/ia32/full-codegen-i...
File src/ia32/full-codegen-ia32.cc (right):

https://chromiumcodereview.appspot.com/9117034/diff/1/src/ia32/full-codegen-i...
src/ia32/full-codegen-ia32.cc:2888: __ JumpIfSmi(eax, &done, Label::kNear);
On 2012/01/25 13:08:48, ulan wrote:
> As discussed offline, it's better to crash if DateField is called not on
JSDate.

I removed all (non-debug-mode) type checks, and checked through all call-sites.
Actually uncovered a hidden bug in the existing Date.UTC.

https://chromiumcodereview.appspot.com/9117034/diff/1/src/ia32/full-codegen-i...
src/ia32/full-codegen-ia32.cc:2951: VisitForStackValue(args->at(0));  // Load
the object.
On 2012/01/25 13:08:48, ulan wrote:
> Is it possible/faster to do 
> VisitForAccumulatorValue(args->at(0)) and then mov ebx, eax?

No, the second Visit can clobber any register. Keep in mind that the full
code-gen implements a stack machine.

https://chromiumcodereview.appspot.com/9117034/diff/1/src/ia32/full-codegen-i...
src/ia32/full-codegen-ia32.cc:2957: __ JumpIfSmi(ebx, &done, Label::kNear);
On 2012/01/25 13:08:48, ulan wrote:
> As discussed offline, it's better to crash if DateField is called not on
JSDate.

Done.

https://chromiumcodereview.appspot.com/9117034/diff/1/src/ia32/full-codegen-i...
src/ia32/full-codegen-ia32.cc:2969: __ RecordWriteField(ebx, JSDate::kYearOffset
+ kPointerSize * index,
On 2012/01/25 13:08:48, ulan wrote:
> Can we avoid the write barrier by making sure that each field is set to either
a
> smi or NaN?

Done.

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

https://chromiumcodereview.appspot.com/9117034/diff/1/src/ia32/lithium-codege...
src/ia32/lithium-codegen-ia32.cc:1289: __ JumpIfSmi(input, &done, Label::kNear);
On 2012/01/25 13:08:48, ulan wrote:
> See comments in full-codegen-ia32.cc

Done.

https://chromiumcodereview.appspot.com/9117034/diff/1/src/objects-printer.cc
File src/objects-printer.cc (right):

https://chromiumcodereview.appspot.com/9117034/diff/1/src/objects-printer.cc#...
src/objects-printer.cc:655: PrintF(out, " - time = %04d/%02d/%02d
%02d:%02d:%02d.%04d\n",
On 2012/01/25 13:08:48, ulan wrote:
> ms is a three digit number, so %04d should be %03d.

Done.

https://chromiumcodereview.appspot.com/9117034/diff/1/src/objects.h
File src/objects.h (right):

https://chromiumcodereview.appspot.com/9117034/diff/1/src/objects.h#newcode5860
src/objects.h:5860: class JSDate: public JSObject {
On 2012/01/25 13:08:48, ulan wrote:
> As we discussed offline, all these fields could be packed into few SMIs and we
> could designate one bit in each SMI as "the date is a NaN" indicator. A
possible
> layout would be:
> 
> [value]: the time value.
> 
> [year_month_day]: SMI that caches the year in bits[31:12], the month in
> bits[11:8], the day in bits[7:3], the is_cache_filled flag in bit[2], and the
> is_nan flag in bit[1].
> 
> [hour_min_sec_ms]: SMI that caches the components similar to [year_month_day].
> 
> Advantages of this layout:
> - the caches can be filled in lazily;
> - less memory usage and CPU cache pressure.
> 
> Disadvantage:
> - need to check is_nan flag in the getters.

Yeah, I don't want to optimize this prematurely. The whole patch is for
benchmark speed, so I'd like to avoid the extra arithmetic for now and see where
we stand.  Frankly, I don't think space usage matters much for dates anyway, not
on benchmarks and even less so in reality. But let's see.

https://chromiumcodereview.appspot.com/9117034/diff/1/src/objects.h#newcode5863
src/objects.h:5863: // [value]: the time value. Transitional, will disappear.
On 2012/01/25 13:08:48, ulan wrote:
> As we discussed offline, the [value] should stay since it's difficult to
> reconstruct it.

Done.

https://chromiumcodereview.appspot.com/9117034/diff/1/src/objects.h#newcode5871
src/objects.h:5871: // [day]: caches hours.
On 2012/01/25 13:08:48, ulan wrote:
> [day] -> [hour] in the comment.

Done. (And for month above.)

LGTM

Expand Messages | Collapse Messages