Factor out common code from platform-specific deoptimization. Fix Deoptimizer not to need to partit…

Factor out common code from platform-specific deoptimization. Fix Deoptimizer not to need to partition functions, but revoke their code before patching, allowing deoptimizing_code_list to be removed from Code; Add DeoptimizeCodeList API to deoptimizer, which works on a ZoneList<Code*>.

BUG=
R=mstarzinger@chromium.org, ulan@chromium.org

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

[ulan, 2013-07-22 09:23:07]

LGTM with comments.

https://codereview.chromium.org/19638014/diff/1/src/deoptimizer.cc
File src/deoptimizer.cc (right):

https://codereview.chromium.org/19638014/diff/1/src/deoptimizer.cc#newcode486
src/deoptimizer.cc:486: codes->at(i)->set_marked_for_deoptimization(true);
Maybe move marking for deoptimization to the places where the code is added to
the list? Then maintain invariant: code is in the list => code is marked for
deopt.

https://codereview.chromium.org/19638014/diff/1/src/mark-compact.cc
File src/mark-compact.cc (right):

https://codereview.chromium.org/19638014/diff/1/src/mark-compact.cc#newcode491
src/mark-compact.cc:491: // The gc_metadata field is used as a linked list of
code to deopt.
Maybe assert that code->gc_metadata() is optimized code or undefined or 0? To
get at leat some check against overlapping usage of the field by code flusher
and deoptimizer.

https://codereview.chromium.org/19638014/diff/1/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/19638014/diff/1/src/objects.cc#newcode11354
src/objects.cc:11354: if (!code->marked_for_deoptimization()) codes.Add(code,
&zone);
code->set_marked_for_deoptimization(true) after adding to the list would make it
more robust against duplicates.

https://codereview.chromium.org/19638014/diff/1/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/19638014/diff/1/src/objects.h#newcode4557
src/objects.h:4557: // unoptimized code. Optimized code can temporarily store
the head of
This comment is out of sync with code.

[titzer, 2013-07-23 12:40:59]

PTAL: I needed to switch to using the type feedback field of the code object
instead of gc_metadata.

https://codereview.chromium.org/19638014/diff/1/src/deoptimizer.cc
File src/deoptimizer.cc (right):

https://codereview.chromium.org/19638014/diff/1/src/deoptimizer.cc#newcode486
src/deoptimizer.cc:486: codes->at(i)->set_marked_for_deoptimization(true);
On 2013/07/22 09:23:08, ulan wrote:
> Maybe move marking for deoptimization to the places where the code is added to
> the list? Then maintain invariant: code is in the list => code is marked for
> deopt.

I wanted to keep the modification of this flag local to the deoptimizer.

https://codereview.chromium.org/19638014/diff/1/src/mark-compact.cc
File src/mark-compact.cc (right):

https://codereview.chromium.org/19638014/diff/1/src/mark-compact.cc#newcode491
src/mark-compact.cc:491: // The gc_metadata field is used as a linked list of
code to deopt.
On 2013/07/22 09:23:08, ulan wrote:
> Maybe assert that code->gc_metadata() is optimized code or undefined or 0? To
> get at leat some check against overlapping usage of the field by code flusher
> and deoptimizer.

I've had to rework this function because I was not able to use the gc_metadata
on Code objects :(

https://codereview.chromium.org/19638014/diff/1/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/19638014/diff/1/src/objects.cc#newcode11354
src/objects.cc:11354: if (!code->marked_for_deoptimization()) codes.Add(code,
&zone);
On 2013/07/22 09:23:08, ulan wrote:
> code->set_marked_for_deoptimization(true) after adding to the list would make
it
> more robust against duplicates.

Done.

https://codereview.chromium.org/19638014/diff/1/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/19638014/diff/1/src/objects.h#newcode4557
src/objects.h:4557: // unoptimized code. Optimized code can temporarily store
the head of
On 2013/07/22 09:23:08, ulan wrote:
> This comment is out of sync with code.

Done.

[Michael Starzinger, 2013-07-24 09:24:57]

LGTM (with comments).

https://codereview.chromium.org/19638014/diff/11001/src/deoptimizer.cc
File src/deoptimizer.cc (right):

https://codereview.chromium.org/19638014/diff/11001/src/deoptimizer.cc#newcod...
src/deoptimizer.cc:346: // TODO(titzer): rewrite to not modify unselected
functions.
I am unsure about this TODO, what does this imply?

https://codereview.chromium.org/19638014/diff/11001/src/deoptimizer.cc#newcod...
src/deoptimizer.cc:472: class DeoptimizeMarkedCodeFilter : public
OptimizedFunctionFilter {
nit: Can we move this up to the other filters so that they are grouped together?

https://codereview.chromium.org/19638014/diff/11001/src/mark-compact.cc
File src/mark-compact.cc (right):

https://codereview.chromium.org/19638014/diff/11001/src/mark-compact.cc#newco...
src/mark-compact.cc:3294: return code->code_to_deoptimize_link() !=
heap()->undefined_value()
It's only valid to ask this question for optimized code, can we add an assert in
that regard?

https://codereview.chromium.org/19638014/diff/11001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/19638014/diff/11001/src/objects-inl.h#newcode...
src/objects-inl.h:5245: return Object::cast(READ_FIELD(this,
kTypeFeedbackInfoOffset));
The "Object::cast" is a NOP as READ_FIELD already gives the correct type, lets
drop it.

[titzer, 2013-07-24 09:54:42]

On 2013/07/24 09:24:57, Michael Starzinger wrote:
> LGTM (with comments).
> 
> https://codereview.chromium.org/19638014/diff/11001/src/deoptimizer.cc
> File src/deoptimizer.cc (right):
> 
>
https://codereview.chromium.org/19638014/diff/11001/src/deoptimizer.cc#newcod...
> src/deoptimizer.cc:346: // TODO(titzer): rewrite to not modify unselected
> functions.
> I am unsure about this TODO, what does this imply?
> 

As discussed over IM, this just means that we walk over the list and only modify
those nodes that point to code functions we select, under the assumption that we
usually won't select a function.

>
https://codereview.chromium.org/19638014/diff/11001/src/deoptimizer.cc#newcod...
> src/deoptimizer.cc:472: class DeoptimizeMarkedCodeFilter : public
> OptimizedFunctionFilter {
> nit: Can we move this up to the other filters so that they are grouped
together?
> 

Done.

> https://codereview.chromium.org/19638014/diff/11001/src/mark-compact.cc
> File src/mark-compact.cc (right):
> 
>
https://codereview.chromium.org/19638014/diff/11001/src/mark-compact.cc#newco...
> src/mark-compact.cc:3294: return code->code_to_deoptimize_link() !=
> heap()->undefined_value()
> It's only valid to ask this question for optimized code, can we add an assert
in
> that regard?

The code_to_deoptimize_link() function has an ASSERT(kind() ==
OPTIMIZED_FUNCTION) inside.

> 
> https://codereview.chromium.org/19638014/diff/11001/src/objects-inl.h
> File src/objects-inl.h (right):
> 
>
https://codereview.chromium.org/19638014/diff/11001/src/objects-inl.h#newcode...
> src/objects-inl.h:5245: return Object::cast(READ_FIELD(this,
> kTypeFeedbackInfoOffset));
> The "Object::cast" is a NOP as READ_FIELD already gives the correct type, lets
> drop it.

Done.

[titzer, 2013-07-24 11:12:26]

Committed patchset #4 manually as r15854 (presubmit successful).