Use optimized code map to cache OSR code.

Use optimized code map to cache OSR code.

R=verwaest@chromium.org

[Michael Starzinger, 2013-12-03 15:27:32]

Not LGTM, adding Ben as a reviewer.

Ben spent several hours making sure that OSR code is _not_ installed in
JSFunction objects. This change completely defeats and breaks that invariant, by
bypassing the choking point that was introduced. I agree that we might want to
cache OSR code, but we don't want to install it in JSFunction objects.

https://codereview.chromium.org/100613004/diff/20001/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):

https://codereview.chromium.org/100613004/diff/20001/src/code-stubs-hydrogen....
src/code-stubs-hydrogen.cc:1195: // case, it's probably a good idea to just
reuse that OSR code right now.
I strongly disagree that this is a good idea! Once Ben's work is done, it will
be incorrect to do that. This will just take whatever code is in the
optimized_map and smash it into the JSFunction. OSR code should not be installed
into JSFunctions at all.

[titzer, 2013-12-03 15:47:02]

On 2013/12/03 15:27:32, Michael Starzinger wrote:
> Not LGTM, adding Ben as a reviewer.
> 
> Ben spent several hours making sure that OSR code is _not_ installed in
> JSFunction objects. This change completely defeats and breaks that invariant,
by
> bypassing the choking point that was introduced. I agree that we might want to
> cache OSR code, but we don't want to install it in JSFunction objects.
> 
>
https://codereview.chromium.org/100613004/diff/20001/src/code-stubs-hydrogen.cc
> File src/code-stubs-hydrogen.cc (right):
> 
>
https://codereview.chromium.org/100613004/diff/20001/src/code-stubs-hydrogen....
> src/code-stubs-hydrogen.cc:1195: // case, it's probably a good idea to just
> reuse that OSR code right now.
> I strongly disagree that this is a good idea! Once Ben's work is done, it will
> be incorrect to do that. This will just take whatever code is in the
> optimized_map and smash it into the JSFunction. OSR code should not be
installed
> into JSFunctions at all.

By the way, I just remembered that we have hydrogenized code that checks the
optimized code map something like FastNewClosureStub; that's bad since even if
you fix it here, that stub might find OSR code.

I'm kind of thinking you might want a separate cache of OSR-compiled code, which
we sort of already have hanging around in the compiler itself in the queues
between the compiler thread and the main thread. I suggest repurposing that to
cache the OSR code for a limited time.

IIRC the optimized code map is cleared by the GC at various times, so the
lifetime of that cache should be similar.

[titzer, 2013-12-03 17:07:16]

On 2013/12/03 15:47:02, titzer wrote:
> On 2013/12/03 15:27:32, Michael Starzinger wrote:
> > Not LGTM, adding Ben as a reviewer.
> > 
> > Ben spent several hours making sure that OSR code is _not_ installed in
> > JSFunction objects. This change completely defeats and breaks that
invariant,
> by
> > bypassing the choking point that was introduced. I agree that we might want
to
> > cache OSR code, but we don't want to install it in JSFunction objects.
> > 
> >
>
https://codereview.chromium.org/100613004/diff/20001/src/code-stubs-hydrogen.cc
> > File src/code-stubs-hydrogen.cc (right):
> > 
> >
>
https://codereview.chromium.org/100613004/diff/20001/src/code-stubs-hydrogen....
> > src/code-stubs-hydrogen.cc:1195: // case, it's probably a good idea to just
> > reuse that OSR code right now.
> > I strongly disagree that this is a good idea! Once Ben's work is done, it
will
> > be incorrect to do that. This will just take whatever code is in the
> > optimized_map and smash it into the JSFunction. OSR code should not be
> installed
> > into JSFunctions at all.
> 
> By the way, I just remembered that we have hydrogenized code that checks the
> optimized code map something like FastNewClosureStub; that's bad since even if
> you fix it here, that stub might find OSR code.
> 
> I'm kind of thinking you might want a separate cache of OSR-compiled code,
which
> we sort of already have hanging around in the compiler itself in the queues
> between the compiler thread and the main thread. I suggest repurposing that to
> cache the OSR code for a limited time.
> 
> IIRC the optimized code map is cleared by the GC at various times, so the
> lifetime of that cache should be similar.

As Michael pointed out, I am retarded. This comment is _in_ the callee of this
function. :)

[Yang, 2013-12-03 17:28:16]

On 2013/12/03 17:07:16, titzer wrote:
> On 2013/12/03 15:47:02, titzer wrote:
> > On 2013/12/03 15:27:32, Michael Starzinger wrote:
> > > Not LGTM, adding Ben as a reviewer.
> > > 
> > > Ben spent several hours making sure that OSR code is _not_ installed in
> > > JSFunction objects. This change completely defeats and breaks that
> invariant,
> > by
> > > bypassing the choking point that was introduced. I agree that we might
want
> to
> > > cache OSR code, but we don't want to install it in JSFunction objects.
> > > 
> > >
> >
>
https://codereview.chromium.org/100613004/diff/20001/src/code-stubs-hydrogen.cc
> > > File src/code-stubs-hydrogen.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/100613004/diff/20001/src/code-stubs-hydrogen....
> > > src/code-stubs-hydrogen.cc:1195: // case, it's probably a good idea to
just
> > > reuse that OSR code right now.
> > > I strongly disagree that this is a good idea! Once Ben's work is done, it
> will
> > > be incorrect to do that. This will just take whatever code is in the
> > > optimized_map and smash it into the JSFunction. OSR code should not be
> > installed
> > > into JSFunctions at all.
> > 
> > By the way, I just remembered that we have hydrogenized code that checks the
> > optimized code map something like FastNewClosureStub; that's bad since even
if
> > you fix it here, that stub might find OSR code.
> > 
> > I'm kind of thinking you might want a separate cache of OSR-compiled code,
> which
> > we sort of already have hanging around in the compiler itself in the queues
> > between the compiler thread and the main thread. I suggest repurposing that
to
> > cache the OSR code for a limited time.
> > 
> > IIRC the optimized code map is cleared by the GC at various times, so the
> > lifetime of that cache should be similar.
> 
> As Michael pointed out, I am retarded. This comment is _in_ the callee of this
> function. :)

Uploaded a new patch set to address the issues.

I'm still using the optimized code map. The rationale is that the OSR buffer in
the optimizing compiler thread does not map shared function info and native
context to code, but function objects to compile jobs. Compile jobs are not yet
generated code and contain keeps a lot of memory alive.

I added a 4th slot to the optimized code map for the osr ast id. I also added
code to the hydrogen stub to ignore those that have an osr ast id set. On
Runtime_CompileForOnStackReplacement I check for cached code before I do
everything. Since I need the osr ast id at that point already, I just removed
the whole thing of using pc_offset as key in RecompileJob.

[Toon Verwaest, 2013-12-04 13:06:25]

Looking good. Just one comment.

https://codereview.chromium.org/100613004/diff/60001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/100613004/diff/60001/src/runtime.cc#newcode8695
src/runtime.cc:8695: function->shared()->InstallFromOptimizedCodeMap(*function,
cached_index);
I'd prefer to have a single choke-point that sets up the function for OSR, that
would be used from both newly OSRd functions and functions OSRd using cached
code. That way it'll be easier to find this place, to change it so that it
doesn't install the OSR code into the function.

[Yang, 2013-12-11 12:32:26]

On 2013/12/04 13:06:25, Toon Verwaest wrote:
> Looking good. Just one comment.
> 
> https://codereview.chromium.org/100613004/diff/60001/src/runtime.cc
> File src/runtime.cc (right):
> 
>
https://codereview.chromium.org/100613004/diff/60001/src/runtime.cc#newcode8695
> src/runtime.cc:8695:
function->shared()->InstallFromOptimizedCodeMap(*function,
> cached_index);
> I'd prefer to have a single choke-point that sets up the function for OSR,
that
> would be used from both newly OSRd functions and functions OSRd using cached
> code. That way it'll be easier to find this place, to change it so that it
> doesn't install the OSR code into the function.

Abandoned in favor of https://codereview.chromium.org/101853003/