Parallel recompilation: fewer handle dereferences and tighter checks.

Parallel recompilation: fewer handle dereferences and tighter checks.


BUG=

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

[Yang, 2013-03-13 13:11:26]

Please take a look.

Review guidance:

- handles.h and handles-inl.h redefine the scope that limits handle
dereferencing. It now also works if parallel recompilation is enabled (see
isolate.cc)
- some of the AllowHandleDereference scopes have been removed. HConstant is most
affected. The idea is to gather all the necessary information at graph creation.
A poor man's copy constructor for HConstant had to be created for
CopyToRepresentation.
- --parallel-recompilation is not compatible with --trace-hydrogen, since we
write into one single file.

[Jakob Kummerow, 2013-03-13 15:50:31]

LGTM with a few minor comments.

https://codereview.chromium.org/12832002/diff/1/src/handles.h
File src/handles.h (right):

https://codereview.chromium.org/12832002/diff/1/src/handles.h#newcode344
src/handles.h:344: enum State { ALLOW, DISALLOW};
nit: space before '}'

https://codereview.chromium.org/12832002/diff/1/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/12832002/diff/1/src/hydrogen-instructions.cc#...
src/hydrogen-instructions.cc:1852: ASSERT(!r.IsNone());
This last ASSERT is unnecessary, you get it for free from set_representation
(called from Initialize()).

https://codereview.chromium.org/12832002/diff/1/src/hydrogen-instructions.cc#...
src/hydrogen-instructions.cc:1858: int32_t integer_value, Representation r, 
Handle<Object> optional_handle)
each argument on its own line please

https://codereview.chromium.org/12832002/diff/1/src/hydrogen-instructions.cc#...
src/hydrogen-instructions.cc:1870: double double_value, Representation r,
Handle<Object> optional_handle)
each argument on its own line please

https://codereview.chromium.org/12832002/diff/1/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/12832002/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:402: bool IsTaggedPrimitive() {
forgot to mark this const too?

https://codereview.chromium.org/12832002/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:3175: bool BooleanValue() { return boolean_value_; }
nit: boolean_value()

https://codereview.chromium.org/12832002/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:3243: bool has_int32_value_ : 1;
I would be nice to replace this with type_from_value_, but I realize that HType
currently can't express the concepts "int32" and "double". :-(

https://codereview.chromium.org/12832002/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:3248: bool is_internalized_string_;  //
TODO(yangguo) make this part of HType
Please move this up to the other has_$FOO_value_ properties.

https://codereview.chromium.org/12832002/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:3249: bool boolean_value_;
Please move this up to the other $FOO_value_ properties.

In fact, please put all four bools next to each other, and restrict them all to
using a single bit, so we can save some memory here.

[Yang, 2013-03-13 16:13:20]

Committed patchset #3 manually as r13935 (presubmit successful).

[Yang, 2013-03-13 17:00:15]

https://codereview.chromium.org/12832002/diff/1/src/handles.h
File src/handles.h (right):

https://codereview.chromium.org/12832002/diff/1/src/handles.h#newcode344
src/handles.h:344: enum State { ALLOW, DISALLOW};
On 2013/03/13 15:50:31, Jakob wrote:
> nit: space before '}'

Done.

https://codereview.chromium.org/12832002/diff/1/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/12832002/diff/1/src/hydrogen-instructions.cc#...
src/hydrogen-instructions.cc:1852: ASSERT(!r.IsNone());
On 2013/03/13 15:50:31, Jakob wrote:
> This last ASSERT is unnecessary, you get it for free from set_representation
> (called from Initialize()).

Done.

https://codereview.chromium.org/12832002/diff/1/src/hydrogen-instructions.cc#...
src/hydrogen-instructions.cc:1858: int32_t integer_value, Representation r, 
Handle<Object> optional_handle)
On 2013/03/13 15:50:31, Jakob wrote:
> each argument on its own line please

Done.

https://codereview.chromium.org/12832002/diff/1/src/hydrogen-instructions.cc#...
src/hydrogen-instructions.cc:1870: double double_value, Representation r,
Handle<Object> optional_handle)
On 2013/03/13 15:50:31, Jakob wrote:
> each argument on its own line please

Done.

https://codereview.chromium.org/12832002/diff/1/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/12832002/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:402: bool IsTaggedPrimitive() {
On 2013/03/13 15:50:31, Jakob wrote:
> forgot to mark this const too?

Done.

https://codereview.chromium.org/12832002/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:3175: bool BooleanValue() { return boolean_value_; }
On 2013/03/13 15:50:31, Jakob wrote:
> nit: boolean_value()

I thought camel case was more appropriate since StringValue() and DoubleValue()
are also simple getters.

https://codereview.chromium.org/12832002/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:3243: bool has_int32_value_ : 1;
On 2013/03/13 15:50:31, Jakob wrote:
> I would be nice to replace this with type_from_value_, but I realize that
HType
> currently can't express the concepts "int32" and "double". :-(

Done.

https://codereview.chromium.org/12832002/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:3248: bool is_internalized_string_;  //
TODO(yangguo) make this part of HType
On 2013/03/13 15:50:31, Jakob wrote:
> Please move this up to the other has_$FOO_value_ properties.

Done.

https://codereview.chromium.org/12832002/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:3249: bool boolean_value_;
On 2013/03/13 15:50:31, Jakob wrote:
> Please move this up to the other $FOO_value_ properties.
> 
> In fact, please put all four bools next to each other, and restrict them all
to
> using a single bit, so we can save some memory here.

Done.