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-thread.cc
> > > File src/optimizing-compiler-thread.cc (right):
> > >
> > >
> >
>
https://codereview.chromium.org/24237009/diff/3001/src/optimizing-compiler-thread.cc#newcode186
> > > 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-thread.cc#newcode304
> > > 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-thread.h
> > > File src/optimizing-compiler-thread.h (right):
> > >
> > >
> >
>
https://codereview.chromium.org/24237009/diff/3001/src/optimizing-compiler-thread.h#newcode122
> > > 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.
https://codereview.chromium.org/24237009/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.
