Disallow dereferencing deferred handles when generating optimized code.

Disallow dereferencing deferred handles when generating optimized code.


R=mvstanton@chromium.org
BUG=

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

[Yang, 2013-04-22 16:27:44]

With parallel recompilation, execution continues between graph creation and code
generation and may have altered the heap. To avoid inconsistencies between graph
creation and code generation, we prohibit the code generation phase to access
objects used in the graph creation phase. This would prevent us from accessing
potentially outdated/inconsistent information. Those objects are kept in
deferred handles, providing us with a nice point to place checks.

Since those objects are handed to the code generation phase for a reason
(usually for embedding into the code), we have to make exceptions in some cases
(using the ALLOW_HANDLE_DEREF macro). In other cases, we circumvent the need to
access handles by prefetching the required information in the graph creation
phase.

Please start with handles.h and handles-inl.h.

[Sven Panne, 2013-04-23 06:42:13]

A few DBCs...

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

https://codereview.chromium.org/14403015/diff/1/src/handles.h#newcode363
src/handles.h:363: HandleDereferenceGuard allow(Isolate::Current(),             
               \
Please do *not* introduce any new Isolate::Current() calls, this function is
really on death row. Adding another parameter to the macro is probably the
easiest way for this, the right Isolate is probably easily available at all call
sites, anyway.

https://codereview.chromium.org/14403015/diff/1/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/14403015/diff/1/src/isolate.cc#newcode511
src/isolate.cc:511: bool Isolate::IsDeferredHandle(Object** location) {
Some comment would be very appropriate here: It is very hard to understand what
the code below is actually doing and why. Even better: Instead of commenting it,
rip it apart into easily understandable methods.

https://codereview.chromium.org/14403015/diff/1/src/isolate.cc#newcode520
src/isolate.cc:520: for (int i = 1; i < blocks->length(); i++) {
Why 1 and not 0? Could be correct, but I don't have a clue.

[mvstanton, 2013-04-23 08:58:50]

Okay, just a few comments. Looking good...I like the reason string for
exceptions to handle deref.

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

https://codereview.chromium.org/14403015/diff/5001/src/hydrogen-instructions....
src/hydrogen-instructions.h:4899:
ASSERT(!constructor->shared()->IsInobjectSlackTrackingInProgress());
I like the assert but how do you know slack tracking must be finished for this
object/map?

https://codereview.chromium.org/14403015/diff/5001/src/ia32/lithium-codegen-i...
File src/ia32/lithium-codegen-ia32.cc (right):

https://codereview.chromium.org/14403015/diff/5001/src/ia32/lithium-codegen-i...
src/ia32/lithium-codegen-ia32.cc:6206: if (!pretenure &&
instr->hydrogen()->has_no_literals()) {
I understand the freezing of information into the hydrogen instruction, like
has_no_literals(). But it's confusing to see that you still make the
shared_info() available too. If it's an unsafe structure to peer into, then why
can you get access to it? It's just so you can push the pointer for the calls
below? In that case I'd rather see the declaration for the local shared_info
removed, and just pass instr()->hydrogen()->shared_info() in the __ push calls
below.

https://codereview.chromium.org/14403015/diff/5001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/14403015/diff/5001/src/isolate.cc#newcode531
src/isolate.cc:531: }
Why can't the check of the first block in the list be integrated with the loop,
which uses code like

int block_size = i == 0 ? deferred->first_block_limit_ : kHandleBlockSize;

and then

"handle < blocks->at(0) + block_size"

is it because at(0) doesn't work for the first, partially full block?

[Yang, 2013-04-23 09:19:09]

Addressed comments.

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

https://codereview.chromium.org/14403015/diff/5001/src/hydrogen-instructions....
src/hydrogen-instructions.h:4899:
ASSERT(!constructor->shared()->IsInobjectSlackTrackingInProgress());
On 2013/04/23 08:58:50, mvstanton wrote:
> I like the assert but how do you know slack tracking must be finished for this
> object/map?

I simply moved this assertion from lithium. The use site in hydrogen.cc makes
sure that slack tracking is completed.

https://codereview.chromium.org/14403015/diff/5001/src/ia32/lithium-codegen-i...
File src/ia32/lithium-codegen-ia32.cc (right):

https://codereview.chromium.org/14403015/diff/5001/src/ia32/lithium-codegen-i...
src/ia32/lithium-codegen-ia32.cc:6206: if (!pretenure &&
instr->hydrogen()->has_no_literals()) {
On 2013/04/23 08:58:50, mvstanton wrote:
> I understand the freezing of information into the hydrogen instruction, like
> has_no_literals(). But it's confusing to see that you still make the
> shared_info() available too. If it's an unsafe structure to peer into, then
why
> can you get access to it? It's just so you can push the pointer for the calls
> below? In that case I'd rather see the declaration for the local shared_info
> removed, and just pass instr()->hydrogen()->shared_info() in the __ push calls
> below.

Done.

https://codereview.chromium.org/14403015/diff/5001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/14403015/diff/5001/src/isolate.cc#newcode531
src/isolate.cc:531: }
On 2013/04/23 08:58:50, mvstanton wrote:
> Why can't the check of the first block in the list be integrated with the
loop,
> which uses code like
> 
> int block_size = i == 0 ? deferred->first_block_limit_ : kHandleBlockSize;
> 
> and then
> 
> "handle < blocks->at(0) + block_size"
> 
> is it because at(0) doesn't work for the first, partially full block?

Works. Done.

[mvstanton, 2013-04-23 09:21:30]

lgtm

[Yang, 2013-04-23 09:23:31]

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