Keep a canonical list of shared function infos.

Keep a canonical list of shared function infos.

Each Script object now keeps a WeakFixedArray of SharedFunctionInfo
objects created from this script.

This way, when compiling a function, we do not create duplicate shared
function info objects when recompiling with either compiler.

This fixes a class of issues in the debugger, where we set break points
on one shared function info, but functions from duplicate shared function
infos are not affected.

LOG=N
BUG=v8:4132
Committed: https://crrev.com/c1669450834436508e0007885eb7ac266cbcf083
Cr-Commit-Position: refs/heads/master@{#29151}

[Yang, 2015-06-17 14:07:06]

yangguo@chromium.org changed reviewers:
+ mstarzinger@chromium.org

[Yang, 2015-06-17 14:07:07]

Michael, please take a look.

Hannes and Toon: this CL increases the size of a vanilla isolate from ~722k to
~765k, but does not affect the context size. However, preventing duplicate
shared function info may save some memory when optimizing or debugging.

[Yang, 2015-06-17 15:04:29]

On 2015/06/17 14:07:07, Yang wrote:
> Michael, please take a look.
> 
> Hannes and Toon: this CL increases the size of a vanilla isolate from ~722k to
> ~765k, but does not affect the context size. However, preventing duplicate
> shared function info may save some memory when optimizing or debugging.

I need to fix this CL: it could happen that an inner function escapes, but the
outer functions including their shared function infos died. Recompiling the
outer functions will create duplicate shared function infos. I think I can solve
this by having each shared function info pointing to its outer shared function
info. That way the escaped inner function keeps the whole chain alive.

[Yang, 2015-06-18 09:19:27]

On 2015/06/17 15:04:29, Yang wrote:
> On 2015/06/17 14:07:07, Yang wrote:
> > Michael, please take a look.
> > 
> > Hannes and Toon: this CL increases the size of a vanilla isolate from ~722k
to
> > ~765k, but does not affect the context size. However, preventing duplicate
> > shared function info may save some memory when optimizing or debugging.
> 
> I need to fix this CL: it could happen that an inner function escapes, but the
> outer functions including their shared function infos died. Recompiling the
> outer functions will create duplicate shared function infos. I think I can
solve
> this by having each shared function info pointing to its outer shared function
> info. That way the escaped inner function keeps the whole chain alive.

Updated with discussed implementation.

[Yang, 2015-06-19 10:14:22]

On 2015/06/18 09:19:27, Yang wrote:
> On 2015/06/17 15:04:29, Yang wrote:
> > On 2015/06/17 14:07:07, Yang wrote:
> > > Michael, please take a look.
> > > 
> > > Hannes and Toon: this CL increases the size of a vanilla isolate from
~722k
> to
> > > ~765k, but does not affect the context size. However, preventing duplicate
> > > shared function info may save some memory when optimizing or debugging.
> > 
> > I need to fix this CL: it could happen that an inner function escapes, but
the
> > outer functions including their shared function infos died. Recompiling the
> > outer functions will create duplicate shared function infos. I think I can
> solve
> > this by having each shared function info pointing to its outer shared
function
> > info. That way the escaped inner function keeps the whole chain alive.
> 
> Updated with discussed implementation.

ping

[Michael Starzinger, 2015-06-19 11:17:08]

LGTM on the implementation with a few comments. About the additional memory
usage, I'll defer to Hannes and/or Toon.

https://codereview.chromium.org/1183733006/diff/60001/src/compiler/ast-graph-...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/1183733006/diff/60001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.cc:1526: // We also have a stack overflow if the
recursive compilation did.
nit: Unrelated to your change, but can we just drop the second sentence of the
comment? Because I still haven't built a test-case for this and am still
surprised that the DCHECK below never hits.

https://codereview.chromium.org/1183733006/diff/60001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1183733006/diff/60001/src/objects.h#newcode6410
src/objects.h:6410: class SharedFunctionInfo;
nit: Can we just move this up to the forward declarations around line 880 at the
top?

https://codereview.chromium.org/1183733006/diff/60001/src/objects.h#newcode6521
src/objects.h:6521: MaybeHandle<SharedFunctionInfo>
FindSharedFunctionInfo(FunctionLiteral* fun);
nit: IMHO this function needs short explaining comment.

https://codereview.chromium.org/1183733006/diff/60001/src/objects.h#newcode6523
src/objects.h:6523: static void AddSharedFunctionInfo(Handle<Script> script,
nit: This shouldn't be needed anymore, correct?

https://codereview.chromium.org/1183733006/diff/60001/src/objects.h#newcode6675
src/objects.h:6675: static void SetScript(Handle<SharedFunctionInfo> shared,
nit: Likewise, can we get a comment?

https://codereview.chromium.org/1183733006/diff/60001/src/objects.h#newcode6722
src/objects.h:6722: DECL_ACCESSORS(inner_functions, Object)
nit: This shouldn't be needed anymore, correct?

https://codereview.chromium.org/1183733006/diff/60001/src/runtime/runtime-fun...
File src/runtime/runtime-function.cc (right):

https://codereview.chromium.org/1183733006/diff/60001/src/runtime/runtime-fun...
src/runtime/runtime-function.cc:235: SharedFunctionInfo::SetScript(
As discussed offline: It might make sense to visually separate this handlified
call from the pure assignment by just moving it out of the block to around line
246 for example. Applies here and to some other call-site as well.

Also: An interesting experiment might be to insert an explicit GC call into
SetScript and see if that flushes out any bugs with SharedFunctionInfo objects
being in an inconsistent state.

[Yang, 2015-06-19 11:26:40]

On 2015/06/19 11:17:08, Michael Starzinger wrote:
> LGTM on the implementation with a few comments. About the additional memory
> usage, I'll defer to Hannes and/or Toon.
> 
>
https://codereview.chromium.org/1183733006/diff/60001/src/compiler/ast-graph-...
> File src/compiler/ast-graph-builder.cc (right):
> 
>
https://codereview.chromium.org/1183733006/diff/60001/src/compiler/ast-graph-...
> src/compiler/ast-graph-builder.cc:1526: // We also have a stack overflow if
the
> recursive compilation did.
> nit: Unrelated to your change, but can we just drop the second sentence of the
> comment? Because I still haven't built a test-case for this and am still
> surprised that the DCHECK below never hits.
> 
> https://codereview.chromium.org/1183733006/diff/60001/src/objects.h
> File src/objects.h (right):
> 
>
https://codereview.chromium.org/1183733006/diff/60001/src/objects.h#newcode6410
> src/objects.h:6410: class SharedFunctionInfo;
> nit: Can we just move this up to the forward declarations around line 880 at
the
> top?
> 
>
https://codereview.chromium.org/1183733006/diff/60001/src/objects.h#newcode6521
> src/objects.h:6521: MaybeHandle<SharedFunctionInfo>
> FindSharedFunctionInfo(FunctionLiteral* fun);
> nit: IMHO this function needs short explaining comment.
> 
>
https://codereview.chromium.org/1183733006/diff/60001/src/objects.h#newcode6523
> src/objects.h:6523: static void AddSharedFunctionInfo(Handle<Script> script,
> nit: This shouldn't be needed anymore, correct?
> 
>
https://codereview.chromium.org/1183733006/diff/60001/src/objects.h#newcode6675
> src/objects.h:6675: static void SetScript(Handle<SharedFunctionInfo> shared,
> nit: Likewise, can we get a comment?
> 
>
https://codereview.chromium.org/1183733006/diff/60001/src/objects.h#newcode6722
> src/objects.h:6722: DECL_ACCESSORS(inner_functions, Object)
> nit: This shouldn't be needed anymore, correct?
> 
>
https://codereview.chromium.org/1183733006/diff/60001/src/runtime/runtime-fun...
> File src/runtime/runtime-function.cc (right):
> 
>
https://codereview.chromium.org/1183733006/diff/60001/src/runtime/runtime-fun...
> src/runtime/runtime-function.cc:235: SharedFunctionInfo::SetScript(
> As discussed offline: It might make sense to visually separate this handlified
> call from the pure assignment by just moving it out of the block to around
line
> 246 for example. Applies here and to some other call-site as well.
> 
> Also: An interesting experiment might be to insert an explicit GC call into
> SetScript and see if that flushes out any bugs with SharedFunctionInfo objects
> being in an inconsistent state.

Addressed comments. I created a test CL
(https://codereview.chromium.org/1183733006/) to run the suggested GC
experiment.

[Yang, 2015-06-19 11:26:46]

The CQ bit was checked by yangguo@chromium.org to run a CQ dry run

[Yang, 2015-06-19 11:26:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/1183733006/#ps100001
(title: "addressed comments")

[commit-bot: I haz the power, 2015-06-19 11:26:56]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1183733006/100001

[commit-bot: I haz the power, 2015-06-19 11:52:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-19 11:52:17]

Dry run: Try jobs failed on following builders:
  v8_linux_nodcheck_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel/build...)

[Yang, 2015-06-19 13:55:02]

The CQ bit was checked by yangguo@chromium.org

[Yang, 2015-06-19 13:55:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/1183733006/#ps120001
(title: "fix test")

[commit-bot: I haz the power, 2015-06-19 13:55:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1183733006/120001

[commit-bot: I haz the power, 2015-06-19 14:40:31]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-06-19 14:40:43]

Patchset 7 (id:??) landed as
https://crrev.com/c1669450834436508e0007885eb7ac266cbcf083
Cr-Commit-Position: refs/heads/master@{#29151}