Less aggressive polling when concurrently compiling for OSR.

Less aggressive polling when concurrently compiling for OSR.

Changes include:
- completed concurrent OSR tasks trigger a stack check interrupt.
- polling for completion is now guarded by a stack check.
- circular buffer for completed OSR tasks instead of list.

R=titzer@chromium.org
BUG=

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

[Yang, 2013-09-20 12:14:19]

Platform ports are still missing, but I would like to have a round of comments
first :)

[Yang, 2013-09-20 13:55:24]

On 2013/09/20 12:14:19, Yang wrote:
> Platform ports are still missing, but I would like to have a round of comments
> first :)

Also completed platform ports.

[titzer, 2013-09-24 09:42:01]

https://codereview.chromium.org/24237009/diff/3001/src/optimizing-compiler-th...
File src/optimizing-compiler-thread.cc (right):

https://codereview.chromium.org/24237009/diff/3001/src/optimizing-compiler-th...
src/optimizing-compiler-thread.cc:186: FlushOutputQueue(false);
Shouldn't you also empty the osr buffer here too?

https://codereview.chromium.org/24237009/diff/3001/src/optimizing-compiler-th...
src/optimizing-compiler-thread.cc:304: delete info;
Maybe you want DisposeOptimizingCompiler(stale, false)? That way, all the
deletes go through the same code path.

https://codereview.chromium.org/24237009/diff/3001/src/optimizing-compiler-th...
File src/optimizing-compiler-thread.h (right):

https://codereview.chromium.org/24237009/diff/3001/src/optimizing-compiler-th...
src/optimizing-compiler-thread.h:122: List<OptimizingCompiler*> osr_candidates_;
First, I am not even sure why you need two buffers here.

Second, It looks like this two buffer scheme can leak OptimizingCompilers. You
are using the osr_candidates_ as a list of the OSR compilation requests. When
the compilation request finishes, it gets moved from the osr_candidates_ to the
osr_buffer_, the latter of which is a fixed-size buffer. But that buffer holds
onto those OptimizingCompilers until they are either removed by
FindReadyOsrCandidate or until they are overwritten by the next AddToOsrBuffer.
That's a problem, since an Optimizing compiler holds onto potentially a lot of
memory--the entire hydrogen and lithium graphs, I think.

[Yang, 2013-09-24 10:07:11]

https://codereview.chromium.org/24237009/diff/3001/src/optimizing-compiler-th...
File src/optimizing-compiler-thread.cc (right):

https://codereview.chromium.org/24237009/diff/3001/src/optimizing-compiler-th...
src/optimizing-compiler-thread.cc:186: FlushOutputQueue(false);
On 2013/09/24 09:42:01, titzer wrote:
> Shouldn't you also empty the osr buffer here too?

This is already done in FlushOutputQueue.

https://codereview.chromium.org/24237009/diff/3001/src/optimizing-compiler-th...
src/optimizing-compiler-thread.cc:304: delete info;
On 2013/09/24 09:42:01, titzer wrote:
> Maybe you want DisposeOptimizingCompiler(stale, false)? That way, all the
> deletes go through the same code path.

Done.

https://codereview.chromium.org/24237009/diff/3001/src/optimizing-compiler-th...
File src/optimizing-compiler-thread.h (right):

https://codereview.chromium.org/24237009/diff/3001/src/optimizing-compiler-th...
src/optimizing-compiler-thread.h:122: List<OptimizingCompiler*> osr_candidates_;
On 2013/09/24 09:42:01, titzer wrote:
> First, I am not even sure why you need two buffers here.
> 
> Second, It looks like this two buffer scheme can leak OptimizingCompilers. You
> are using the osr_candidates_ as a list of the OSR compilation requests. When
> the compilation request finishes, it gets moved from the osr_candidates_ to
the
> osr_buffer_, the latter of which is a fixed-size buffer. But that buffer holds
> onto those OptimizingCompilers until they are either removed by
> FindReadyOsrCandidate or until they are overwritten by the next
AddToOsrBuffer.
> That's a problem, since an Optimizing compiler holds onto potentially a lot of
> memory--the entire hydrogen and lithium graphs, I think.

osr_candidates_ simply mirrors OSR jobs that are either still in the
input_queue_, being processed by CompileNext(), or waiting in the output_queue_.
Once a job ends up in the osr_buffer_, it's removed from osr_candidates_.
Assuming that the recompile loop doesn't block somewhere, it always gets removed
from osr_candidates_ eventually. It's used to check whether we already have a
certain OSR job (function x ast id), since the back edge state is not 100%
reliable for this purpose.

The problem of osr_buffer_ holding onto OptimizingCompiler objects is unrelated
to this. It seems unavoidable since we don't know when or whether a completed
OSR job actually gets used for OSR entry, so we have to have a number of recent
ones waiting. The best solution I could think of for now is to somehow install
hooks at the point when we enter optimized code for a function to remove all
other waiting OSR jobs for that function. For now all we do is flushing
osr_buffer_ at context (V8::Context) change.

[Yang, 2013-09-24 10:34:15]

On 2013/09/24 10:07:11, Yang wrote:
>
https://codereview.chromium.org/24237009/diff/3001/src/optimizing-compiler-th...
> File src/optimizing-compiler-thread.cc (right):
> 
>
https://codereview.chromium.org/24237009/diff/3001/src/optimizing-compiler-th...
> src/optimizing-compiler-thread.cc:186: FlushOutputQueue(false);
> On 2013/09/24 09:42:01, titzer wrote:
> > Shouldn't you also empty the osr buffer here too?
> 
> This is already done in FlushOutputQueue.
> 
>
https://codereview.chromium.org/24237009/diff/3001/src/optimizing-compiler-th...
> src/optimizing-compiler-thread.cc:304: delete info;
> On 2013/09/24 09:42:01, titzer wrote:
> > Maybe you want DisposeOptimizingCompiler(stale, false)? That way, all the
> > deletes go through the same code path.
> 
> Done.
> 
>
https://codereview.chromium.org/24237009/diff/3001/src/optimizing-compiler-th...
> File src/optimizing-compiler-thread.h (right):
> 
>
https://codereview.chromium.org/24237009/diff/3001/src/optimizing-compiler-th...
> src/optimizing-compiler-thread.h:122: List<OptimizingCompiler*>
osr_candidates_;
> On 2013/09/24 09:42:01, titzer wrote:
> > First, I am not even sure why you need two buffers here.
> > 
> > Second, It looks like this two buffer scheme can leak OptimizingCompilers.
You
> > are using the osr_candidates_ as a list of the OSR compilation requests.
When
> > the compilation request finishes, it gets moved from the osr_candidates_ to
> the
> > osr_buffer_, the latter of which is a fixed-size buffer. But that buffer
holds
> > onto those OptimizingCompilers until they are either removed by
> > FindReadyOsrCandidate or until they are overwritten by the next
> AddToOsrBuffer.
> > That's a problem, since an Optimizing compiler holds onto potentially a lot
of
> > memory--the entire hydrogen and lithium graphs, I think.
> 
> osr_candidates_ simply mirrors OSR jobs that are either still in the
> input_queue_, being processed by CompileNext(), or waiting in the
output_queue_.
> Once a job ends up in the osr_buffer_, it's removed from osr_candidates_.
> Assuming that the recompile loop doesn't block somewhere, it always gets
removed
> from osr_candidates_ eventually. It's used to check whether we already have a
> certain OSR job (function x ast id), since the back edge state is not 100%
> reliable for this purpose.
> 
> The problem of osr_buffer_ holding onto OptimizingCompiler objects is
unrelated
> to this. It seems unavoidable since we don't know when or whether a completed
> OSR job actually gets used for OSR entry, so we have to have a number of
recent
> ones waiting. The best solution I could think of for now is to somehow install
> hooks at the point when we enter optimized code for a function to remove all
> other waiting OSR jobs for that function. For now all we do is flushing
> osr_buffer_ at context (V8::Context) change.

I just created another CL to address this problem: on last-resort GC (called
from V8 or by the embedder via LowMemoryNotification), we don't care about
performance anymore, so we can flush the optimizing compiler's queues and
buffers.

https://codereview.chromium.org/24145011/

[titzer, 2013-09-24 10:48:12]

On 2013/09/24 10:07:11, Yang wrote:
>
https://codereview.chromium.org/24237009/diff/3001/src/optimizing-compiler-th...
> File src/optimizing-compiler-thread.cc (right):
> 
>
https://codereview.chromium.org/24237009/diff/3001/src/optimizing-compiler-th...
> src/optimizing-compiler-thread.cc:186: FlushOutputQueue(false);
> On 2013/09/24 09:42:01, titzer wrote:
> > Shouldn't you also empty the osr buffer here too?
> 
> This is already done in FlushOutputQueue.
> 
>
https://codereview.chromium.org/24237009/diff/3001/src/optimizing-compiler-th...
> src/optimizing-compiler-thread.cc:304: delete info;
> On 2013/09/24 09:42:01, titzer wrote:
> > Maybe you want DisposeOptimizingCompiler(stale, false)? That way, all the
> > deletes go through the same code path.
> 
> Done.
> 
>
https://codereview.chromium.org/24237009/diff/3001/src/optimizing-compiler-th...
> File src/optimizing-compiler-thread.h (right):
> 
>
https://codereview.chromium.org/24237009/diff/3001/src/optimizing-compiler-th...
> src/optimizing-compiler-thread.h:122: List<OptimizingCompiler*>
osr_candidates_;
> On 2013/09/24 09:42:01, titzer wrote:
> > First, I am not even sure why you need two buffers here.
> > 
> > Second, It looks like this two buffer scheme can leak OptimizingCompilers.
You
> > are using the osr_candidates_ as a list of the OSR compilation requests.
When
> > the compilation request finishes, it gets moved from the osr_candidates_ to
> the
> > osr_buffer_, the latter of which is a fixed-size buffer. But that buffer
holds
> > onto those OptimizingCompilers until they are either removed by
> > FindReadyOsrCandidate or until they are overwritten by the next
> AddToOsrBuffer.
> > That's a problem, since an Optimizing compiler holds onto potentially a lot
of
> > memory--the entire hydrogen and lithium graphs, I think.
> 
> osr_candidates_ simply mirrors OSR jobs that are either still in the
> input_queue_, being processed by CompileNext(), or waiting in the
output_queue_.
> Once a job ends up in the osr_buffer_, it's removed from osr_candidates_.
> Assuming that the recompile loop doesn't block somewhere, it always gets
removed
> from osr_candidates_ eventually. It's used to check whether we already have a
> certain OSR job (function x ast id), since the back edge state is not 100%
> reliable for this purpose.

I think you can do this with just one list of the OSR-related compilation
requests. They just stay in the list until they are pulled out by a successful
FindReadyOsrCandidate.

> 
> The problem of osr_buffer_ holding onto OptimizingCompiler objects is
unrelated
> to this. It seems unavoidable since we don't know when or whether a completed
> OSR job actually gets used for OSR entry, so we have to have a number of
recent
> ones waiting. The best solution I could think of for now is to somehow install
> hooks at the point when we enter optimized code for a function to remove all
> other waiting OSR jobs for that function. For now all we do is flushing
> osr_buffer_ at context (V8::Context) change.

Yes, it's a separate problem that is independent of whether there are two
buffers. There either needs to be some kind of aging mechanism or timeout or
something. You suggested using the low memory notification; that only works if
the system is running out of memory. If it isn't, then these other OSR compiles
will just lay around forever taking up memory. We need to come up with a
solution for that...

[Yang, 2013-09-24 11:11:26]

On 2013/09/24 10:48:12, titzer wrote:
> On 2013/09/24 10:07:11, Yang wrote:
> >
>
https://codereview.chromium.org/24237009/diff/3001/src/optimizing-compiler-th...
> > File src/optimizing-compiler-thread.cc (right):
> > 
> >
>
https://codereview.chromium.org/24237009/diff/3001/src/optimizing-compiler-th...
> > src/optimizing-compiler-thread.cc:186: FlushOutputQueue(false);
> > On 2013/09/24 09:42:01, titzer wrote:
> > > Shouldn't you also empty the osr buffer here too?
> > 
> > This is already done in FlushOutputQueue.
> > 
> >
>
https://codereview.chromium.org/24237009/diff/3001/src/optimizing-compiler-th...
> > src/optimizing-compiler-thread.cc:304: delete info;
> > On 2013/09/24 09:42:01, titzer wrote:
> > > Maybe you want DisposeOptimizingCompiler(stale, false)? That way, all the
> > > deletes go through the same code path.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/24237009/diff/3001/src/optimizing-compiler-th...
> > File src/optimizing-compiler-thread.h (right):
> > 
> >
>
https://codereview.chromium.org/24237009/diff/3001/src/optimizing-compiler-th...
> > src/optimizing-compiler-thread.h:122: List<OptimizingCompiler*>
> osr_candidates_;
> > On 2013/09/24 09:42:01, titzer wrote:
> > > First, I am not even sure why you need two buffers here.
> > > 
> > > Second, It looks like this two buffer scheme can leak OptimizingCompilers.
> You
> > > are using the osr_candidates_ as a list of the OSR compilation requests.
> When
> > > the compilation request finishes, it gets moved from the osr_candidates_
to
> > the
> > > osr_buffer_, the latter of which is a fixed-size buffer. But that buffer
> holds
> > > onto those OptimizingCompilers until they are either removed by
> > > FindReadyOsrCandidate or until they are overwritten by the next
> > AddToOsrBuffer.
> > > That's a problem, since an Optimizing compiler holds onto potentially a
lot
> of
> > > memory--the entire hydrogen and lithium graphs, I think.
> > 
> > osr_candidates_ simply mirrors OSR jobs that are either still in the
> > input_queue_, being processed by CompileNext(), or waiting in the
> output_queue_.
> > Once a job ends up in the osr_buffer_, it's removed from osr_candidates_.
> > Assuming that the recompile loop doesn't block somewhere, it always gets
> removed
> > from osr_candidates_ eventually. It's used to check whether we already have
a
> > certain OSR job (function x ast id), since the back edge state is not 100%
> > reliable for this purpose.
> 
> I think you can do this with just one list of the OSR-related compilation
> requests. They just stay in the list until they are pulled out by a successful
> FindReadyOsrCandidate.

But in that case I would have to remove elements from that list when we either
successfully install on FindReadyOsrCandidate or when we discard when the cyclic
buffer entry is overwritten.

In the current way we have a single choke point, which is when we move completed
OSR jobs from the output_queue_ to the cyclic buffer.

If you mean that osr_buffer_ and osr_candidates_ can be merged into one list,
then how would we know whether a job in the list is complete?

> 
> > 
> > The problem of osr_buffer_ holding onto OptimizingCompiler objects is
> unrelated
> > to this. It seems unavoidable since we don't know when or whether a
completed
> > OSR job actually gets used for OSR entry, so we have to have a number of
> recent
> > ones waiting. The best solution I could think of for now is to somehow
install
> > hooks at the point when we enter optimized code for a function to remove all
> > other waiting OSR jobs for that function. For now all we do is flushing
> > osr_buffer_ at context (V8::Context) change.
> 
> Yes, it's a separate problem that is independent of whether there are two
> buffers. There either needs to be some kind of aging mechanism or timeout or
> something. You suggested using the low memory notification; that only works if
> the system is running out of memory. If it isn't, then these other OSR
compiles
> will just lay around forever taking up memory. We need to come up with a
> solution for that...

[Yang, 2013-09-24 13:33:05]

On 2013/09/24 11:11:26, Yang wrote:
> On 2013/09/24 10:48:12, titzer wrote:
> > On 2013/09/24 10:07:11, Yang wrote:
> > >
> >
>
https://codereview.chromium.org/24237009/diff/3001/src/optimizing-compiler-th...
> > > File src/optimizing-compiler-thread.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/24237009/diff/3001/src/optimizing-compiler-th...
> > > src/optimizing-compiler-thread.cc:186: FlushOutputQueue(false);
> > > On 2013/09/24 09:42:01, titzer wrote:
> > > > Shouldn't you also empty the osr buffer here too?
> > > 
> > > This is already done in FlushOutputQueue.
> > > 
> > >
> >
>
https://codereview.chromium.org/24237009/diff/3001/src/optimizing-compiler-th...
> > > src/optimizing-compiler-thread.cc:304: delete info;
> > > On 2013/09/24 09:42:01, titzer wrote:
> > > > Maybe you want DisposeOptimizingCompiler(stale, false)? That way, all
the
> > > > deletes go through the same code path.
> > > 
> > > Done.
> > > 
> > >
> >
>
https://codereview.chromium.org/24237009/diff/3001/src/optimizing-compiler-th...
> > > File src/optimizing-compiler-thread.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/24237009/diff/3001/src/optimizing-compiler-th...
> > > src/optimizing-compiler-thread.h:122: List<OptimizingCompiler*>
> > osr_candidates_;
> > > On 2013/09/24 09:42:01, titzer wrote:
> > > > First, I am not even sure why you need two buffers here.
> > > > 
> > > > Second, It looks like this two buffer scheme can leak
OptimizingCompilers.
> > You
> > > > are using the osr_candidates_ as a list of the OSR compilation requests.
> > When
> > > > the compilation request finishes, it gets moved from the osr_candidates_
> to
> > > the
> > > > osr_buffer_, the latter of which is a fixed-size buffer. But that buffer
> > holds
> > > > onto those OptimizingCompilers until they are either removed by
> > > > FindReadyOsrCandidate or until they are overwritten by the next
> > > AddToOsrBuffer.
> > > > That's a problem, since an Optimizing compiler holds onto potentially a
> lot
> > of
> > > > memory--the entire hydrogen and lithium graphs, I think.
> > > 
> > > osr_candidates_ simply mirrors OSR jobs that are either still in the
> > > input_queue_, being processed by CompileNext(), or waiting in the
> > output_queue_.
> > > Once a job ends up in the osr_buffer_, it's removed from osr_candidates_.
> > > Assuming that the recompile loop doesn't block somewhere, it always gets
> > removed
> > > from osr_candidates_ eventually. It's used to check whether we already
have
> a
> > > certain OSR job (function x ast id), since the back edge state is not 100%
> > > reliable for this purpose.
> > 
> > I think you can do this with just one list of the OSR-related compilation
> > requests. They just stay in the list until they are pulled out by a
successful
> > FindReadyOsrCandidate.
> 
> But in that case I would have to remove elements from that list when we either
> successfully install on FindReadyOsrCandidate or when we discard when the
cyclic
> buffer entry is overwritten.
> 
> In the current way we have a single choke point, which is when we move
completed
> OSR jobs from the output_queue_ to the cyclic buffer.
> 
> If you mean that osr_buffer_ and osr_candidates_ can be merged into one list,
> then how would we know whether a job in the list is complete?
> 
> > 
> > > 
> > > The problem of osr_buffer_ holding onto OptimizingCompiler objects is
> > unrelated
> > > to this. It seems unavoidable since we don't know when or whether a
> completed
> > > OSR job actually gets used for OSR entry, so we have to have a number of
> > recent
> > > ones waiting. The best solution I could think of for now is to somehow
> install
> > > hooks at the point when we enter optimized code for a function to remove
all
> > > other waiting OSR jobs for that function. For now all we do is flushing
> > > osr_buffer_ at context (V8::Context) change.
> > 
> > Yes, it's a separate problem that is independent of whether there are two
> > buffers. There either needs to be some kind of aging mechanism or timeout or
> > something. You suggested using the low memory notification; that only works
if
> > the system is running out of memory. If it isn't, then these other OSR
> compiles
> > will just lay around forever taking up memory. We need to come up with a
> > solution for that...

Changed according to comment.

[titzer, 2013-09-25 07:58:47]

On 2013/09/24 13:33:05, Yang wrote:
> On 2013/09/24 11:11:26, Yang wrote:
> > On 2013/09/24 10:48:12, titzer wrote:
> > > On 2013/09/24 10:07:11, Yang wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/24237009/diff/3001/src/optimizing-compiler-th...
> > > > File src/optimizing-compiler-thread.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/24237009/diff/3001/src/optimizing-compiler-th...
> > > > src/optimizing-compiler-thread.cc:186: FlushOutputQueue(false);
> > > > On 2013/09/24 09:42:01, titzer wrote:
> > > > > Shouldn't you also empty the osr buffer here too?
> > > > 
> > > > This is already done in FlushOutputQueue.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/24237009/diff/3001/src/optimizing-compiler-th...
> > > > src/optimizing-compiler-thread.cc:304: delete info;
> > > > On 2013/09/24 09:42:01, titzer wrote:
> > > > > Maybe you want DisposeOptimizingCompiler(stale, false)? That way, all
> the
> > > > > deletes go through the same code path.
> > > > 
> > > > Done.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/24237009/diff/3001/src/optimizing-compiler-th...
> > > > File src/optimizing-compiler-thread.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/24237009/diff/3001/src/optimizing-compiler-th...
> > > > src/optimizing-compiler-thread.h:122: List<OptimizingCompiler*>
> > > osr_candidates_;
> > > > On 2013/09/24 09:42:01, titzer wrote:
> > > > > First, I am not even sure why you need two buffers here.
> > > > > 
> > > > > Second, It looks like this two buffer scheme can leak
> OptimizingCompilers.
> > > You
> > > > > are using the osr_candidates_ as a list of the OSR compilation
requests.
> > > When
> > > > > the compilation request finishes, it gets moved from the
osr_candidates_
> > to
> > > > the
> > > > > osr_buffer_, the latter of which is a fixed-size buffer. But that
buffer
> > > holds
> > > > > onto those OptimizingCompilers until they are either removed by
> > > > > FindReadyOsrCandidate or until they are overwritten by the next
> > > > AddToOsrBuffer.
> > > > > That's a problem, since an Optimizing compiler holds onto potentially
a
> > lot
> > > of
> > > > > memory--the entire hydrogen and lithium graphs, I think.
> > > > 
> > > > osr_candidates_ simply mirrors OSR jobs that are either still in the
> > > > input_queue_, being processed by CompileNext(), or waiting in the
> > > output_queue_.
> > > > Once a job ends up in the osr_buffer_, it's removed from
osr_candidates_.
> > > > Assuming that the recompile loop doesn't block somewhere, it always gets
> > > removed
> > > > from osr_candidates_ eventually. It's used to check whether we already
> have
> > a
> > > > certain OSR job (function x ast id), since the back edge state is not
100%
> > > > reliable for this purpose.
> > > 
> > > I think you can do this with just one list of the OSR-related compilation
> > > requests. They just stay in the list until they are pulled out by a
> successful
> > > FindReadyOsrCandidate.
> > 
> > But in that case I would have to remove elements from that list when we
either
> > successfully install on FindReadyOsrCandidate or when we discard when the
> cyclic
> > buffer entry is overwritten.
> > 
> > In the current way we have a single choke point, which is when we move
> completed
> > OSR jobs from the output_queue_ to the cyclic buffer.
> > 
> > If you mean that osr_buffer_ and osr_candidates_ can be merged into one
list,
> > then how would we know whether a job in the list is complete?
> > 
> > > 
> > > > 
> > > > The problem of osr_buffer_ holding onto OptimizingCompiler objects is
> > > unrelated
> > > > to this. It seems unavoidable since we don't know when or whether a
> > completed
> > > > OSR job actually gets used for OSR entry, so we have to have a number of
> > > recent
> > > > ones waiting. The best solution I could think of for now is to somehow
> > install
> > > > hooks at the point when we enter optimized code for a function to remove
> all
> > > > other waiting OSR jobs for that function. For now all we do is flushing
> > > > osr_buffer_ at context (V8::Context) change.
> > > 
> > > Yes, it's a separate problem that is independent of whether there are two
> > > buffers. There either needs to be some kind of aging mechanism or timeout
or
> > > something. You suggested using the low memory notification; that only
works
> if
> > > the system is running out of memory. If it isn't, then these other OSR
> > compiles
> > > will just lay around forever taking up memory. We need to come up with a
> > > solution for that...
> 
> Changed according to comment.

LGTM, but please put in a specific TODO for the memory leak issue.

[Yang, 2013-09-25 08:08:12]

On 2013/09/25 07:58:47, titzer wrote:
> On 2013/09/24 13:33:05, Yang wrote:
> > On 2013/09/24 11:11:26, Yang wrote:
> > > On 2013/09/24 10:48:12, titzer wrote:
> > > > On 2013/09/24 10:07:11, Yang wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/24237009/diff/3001/src/optimizing-compiler-th...
> > > > > File src/optimizing-compiler-thread.cc (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/24237009/diff/3001/src/optimizing-compiler-th...
> > > > > src/optimizing-compiler-thread.cc:186: FlushOutputQueue(false);
> > > > > On 2013/09/24 09:42:01, titzer wrote:
> > > > > > Shouldn't you also empty the osr buffer here too?
> > > > > 
> > > > > This is already done in FlushOutputQueue.
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/24237009/diff/3001/src/optimizing-compiler-th...
> > > > > src/optimizing-compiler-thread.cc:304: delete info;
> > > > > On 2013/09/24 09:42:01, titzer wrote:
> > > > > > Maybe you want DisposeOptimizingCompiler(stale, false)? That way,
all
> > the
> > > > > > deletes go through the same code path.
> > > > > 
> > > > > Done.
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/24237009/diff/3001/src/optimizing-compiler-th...
> > > > > File src/optimizing-compiler-thread.h (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/24237009/diff/3001/src/optimizing-compiler-th...
> > > > > src/optimizing-compiler-thread.h:122: List<OptimizingCompiler*>
> > > > osr_candidates_;
> > > > > On 2013/09/24 09:42:01, titzer wrote:
> > > > > > First, I am not even sure why you need two buffers here.
> > > > > > 
> > > > > > Second, It looks like this two buffer scheme can leak
> > OptimizingCompilers.
> > > > You
> > > > > > are using the osr_candidates_ as a list of the OSR compilation
> requests.
> > > > When
> > > > > > the compilation request finishes, it gets moved from the
> osr_candidates_
> > > to
> > > > > the
> > > > > > osr_buffer_, the latter of which is a fixed-size buffer. But that
> buffer
> > > > holds
> > > > > > onto those OptimizingCompilers until they are either removed by
> > > > > > FindReadyOsrCandidate or until they are overwritten by the next
> > > > > AddToOsrBuffer.
> > > > > > That's a problem, since an Optimizing compiler holds onto
potentially
> a
> > > lot
> > > > of
> > > > > > memory--the entire hydrogen and lithium graphs, I think.
> > > > > 
> > > > > osr_candidates_ simply mirrors OSR jobs that are either still in the
> > > > > input_queue_, being processed by CompileNext(), or waiting in the
> > > > output_queue_.
> > > > > Once a job ends up in the osr_buffer_, it's removed from
> osr_candidates_.
> > > > > Assuming that the recompile loop doesn't block somewhere, it always
gets
> > > > removed
> > > > > from osr_candidates_ eventually. It's used to check whether we already
> > have
> > > a
> > > > > certain OSR job (function x ast id), since the back edge state is not
> 100%
> > > > > reliable for this purpose.
> > > > 
> > > > I think you can do this with just one list of the OSR-related
compilation
> > > > requests. They just stay in the list until they are pulled out by a
> > successful
> > > > FindReadyOsrCandidate.
> > > 
> > > But in that case I would have to remove elements from that list when we
> either
> > > successfully install on FindReadyOsrCandidate or when we discard when the
> > cyclic
> > > buffer entry is overwritten.
> > > 
> > > In the current way we have a single choke point, which is when we move
> > completed
> > > OSR jobs from the output_queue_ to the cyclic buffer.
> > > 
> > > If you mean that osr_buffer_ and osr_candidates_ can be merged into one
> list,
> > > then how would we know whether a job in the list is complete?
> > > 
> > > > 
> > > > > 
> > > > > The problem of osr_buffer_ holding onto OptimizingCompiler objects is
> > > > unrelated
> > > > > to this. It seems unavoidable since we don't know when or whether a
> > > completed
> > > > > OSR job actually gets used for OSR entry, so we have to have a number
of
> > > > recent
> > > > > ones waiting. The best solution I could think of for now is to somehow
> > > install
> > > > > hooks at the point when we enter optimized code for a function to
remove
> > all
> > > > > other waiting OSR jobs for that function. For now all we do is
flushing
> > > > > osr_buffer_ at context (V8::Context) change.
> > > > 
> > > > Yes, it's a separate problem that is independent of whether there are
two
> > > > buffers. There either needs to be some kind of aging mechanism or
timeout
> or
> > > > something. You suggested using the low memory notification; that only
> works
> > if
> > > > the system is running out of memory. If it isn't, then these other OSR
> > > compiles
> > > > will just lay around forever taking up memory. We need to come up with a
> > > > solution for that...
> > 
> > Changed according to comment.
> 
> LGTM, but please put in a specific TODO for the memory leak issue.

added todo.

[Yang, 2013-09-25 08:26:28]

Committed patchset #6 manually as r16934 (presubmit successful).