[TypeFeedbackVector] Root feedback vectors at function literal site.

[TypeFeedbackVector] Root feedback vectors at function literal site.

TypeFeedbackVectors are strongly rooted by a closure. However, in modern
JavaScript closures are created and abandoned more freely. An important
closure may not be present in the root-set at time of garbage collection,
even though we've cached optimized code and use it regularly. For
example, consider leaf functions in an event dispatching system. They may
well be "hot," but tragically non-present when we collect the heap.

Until now, we've relied on a weak root to cache the feedback vector in
this case. Since there is no way to signal intent or relative importance,
this weak root is as susceptible to clearing as any other weak root at
garbage collection time.

Meanwhile, the feedback vector has become more important. All of our
ICs store their data there. Literal and regex boilerplates are stored there.
If we lose the vector, then we not only lose optimized code built from
it, we also lose the very feedback which allowed us to create that optimized
code. Therefore it's vital to express that dependency through the root
set.

This CL does this by creating a strong link to a feedback
vector at the instantiation site of the function closure.
This instantiation site is in the code and feedback vector
of the outer closure.

BUG=v8:5456
Review-Url: https://codereview.chromium.org/2674593003
Cr-Commit-Position: refs/heads/master@{#42953}
Committed: https://chromium.googlesource.com/v8/v8/+/aea3ce3df3dc8838940e5db74c9d459a3ba999e3

[mvstanton, 2017-02-02 10:57:24]

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

[commit-bot: I haz the power, 2017-02-02 10:57:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-02 10:59:05]

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

[commit-bot: I haz the power, 2017-02-02 10:59:06]

Dry run: Try jobs failed on following builders:
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/20321)
  v8_linux64_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_verify_csa_rel_n...)
  v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)
  v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)
  v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
  v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/20281)
  v8_linux_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_verify_csa_rel_ng/...)

[mvstanton, 2017-02-02 11:42:12]

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

[commit-bot: I haz the power, 2017-02-02 11:42:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-02 11:45:46]

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

[commit-bot: I haz the power, 2017-02-02 11:45:47]

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

[mvstanton, 2017-02-02 12:05:26]

mvstanton@chromium.org changed reviewers:
+ bmeurer@chromium.org, mstarzinger@chromium.org, yangguo@chromium.org

[mvstanton, 2017-02-02 12:05:26]

mvstanton@chromium.org changed required reviewers:
+ bmeurer@chromium.org, mstarzinger@chromium.org

[mvstanton, 2017-02-02 12:05:27]

Hi guys,
Benedikt - can you review the whole thing. Much is simpler than last time,
though the script and eval caches are new. That cache was "copy-pasted" from the
OptimizedCodeMap to prevent bugs.

Michi - the src/heap change, and any other spots you find interesting.

Yang - my liveedit solution is much simpler now! Have a look at that.

Thanks all,
--Michael

ps - I have a snapshot failure on arm/mips, debugging that, but I don't think
it'll change the design.

[Benedikt Meurer, 2017-02-02 12:47:03]

Leaving compiler and compilation cache review to Michi. Otherwise LGTM from my
side.

https://codereview.chromium.org/2674593003/diff/40001/src/handles-inl.h
File src/handles-inl.h (left):

https://codereview.chromium.org/2674593003/diff/40001/src/handles-inl.h#oldco...
src/handles-inl.h:97: 
Nit: Can you undo the change to this file?

[Michael Starzinger, 2017-02-02 13:42:08]

LGTM on compiler changes. If you want a more thorough look at the compilation
cache changes, then I have to defer to Toon, as I am not familiar with the code.

https://codereview.chromium.org/2674593003/diff/40001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2674593003/diff/40001/src/objects.cc#newcode1...
src/objects.cc:17567: static int SearchLiteralsMapEntry(CompilationCacheTable*
cache, int cache_entry,
nit: Use anonymous namespace instead of static. I think if we move the function
up a couple of lines, we don't need the forward declarations above. WDYT?

https://codereview.chromium.org/2674593003/diff/40001/src/objects.cc#newcode1...
src/objects.cc:17678: // DisallowHeapAllocation no_allocation_scope;
Looks like a left-over. As discussed offline, the shape of the cache table
should remain unchanged by {AddToLiteralsMap}, so the derived {entry} should
still be valid. But I am not an expert on these cache table.

https://codereview.chromium.org/2674593003/diff/40001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2674593003/diff/40001/src/objects.h#newcode8744
src/objects.h:8744: SharedFunctionInfo* info() const { return info_; }
nit: s/info/shared/

https://codereview.chromium.org/2674593003/diff/40001/src/objects.h#newcode8747
src/objects.h:8747: bool has_info() const { return info_ != nullptr; }
nit: s/info/shared/

https://codereview.chromium.org/2674593003/diff/40001/src/objects.h#newcode8751
src/objects.h:8751: SharedFunctionInfo* info_;
nit: s/info/shared/

[Yang, 2017-02-02 14:11:33]

liveedit lgtm.

[mvstanton, 2017-02-02 14:44:46]

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

[commit-bot: I haz the power, 2017-02-02 14:44:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-02 14:50:57]

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

[commit-bot: I haz the power, 2017-02-02 14:50:58]

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

[mvstanton, 2017-02-02 15:20:17]

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

[commit-bot: I haz the power, 2017-02-02 15:20:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-02 15:41:44]

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

[commit-bot: I haz the power, 2017-02-02 15:41:45]

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

[mvstanton, 2017-02-03 12:01:42]

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

[commit-bot: I haz the power, 2017-02-03 12:01:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mvstanton, 2017-02-03 12:15:35]

The CQ bit was checked by mvstanton@chromium.org

[mvstanton, 2017-02-03 12:15:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, mstarzinger@chromium.org, yangguo@chromium.org
Link to the patchset: https://codereview.chromium.org/2674593003/#ps120001
(title: "REBASE+ports update.")

[commit-bot: I haz the power, 2017-02-03 12:15:44]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mvstanton, 2017-02-03 12:16:37]

Thanks everyone! Landing...
--Michael

https://codereview.chromium.org/2674593003/diff/40001/src/debug/liveedit.cc
File src/debug/liveedit.cc (right):

https://codereview.chromium.org/2674593003/diff/40001/src/debug/liveedit.cc#n...
src/debug/liveedit.cc:800: static void IterateAllJSFunctions(Heap* heap,
Visitor* visitor) {
dead code, remove.

https://codereview.chromium.org/2674593003/diff/40001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2674593003/diff/40001/src/objects.cc#newcode1...
src/objects.cc:17567: static int SearchLiteralsMapEntry(CompilationCacheTable*
cache, int cache_entry,
On 2017/02/02 13:42:07, Michael Starzinger wrote:
> nit: Use anonymous namespace instead of static. I think if we move the
function
> up a couple of lines, we don't need the forward declarations above. WDYT?

Excellent, I did both.

https://codereview.chromium.org/2674593003/diff/40001/src/objects.cc#newcode1...
src/objects.cc:17678: // DisallowHeapAllocation no_allocation_scope;
On 2017/02/02 13:42:07, Michael Starzinger wrote:
> Looks like a left-over. As discussed offline, the shape of the cache table
> should remain unchanged by {AddToLiteralsMap}, so the derived {entry} should
> still be valid. But I am not an expert on these cache table.

Thanks, I've added a comment about this - good point!

https://codereview.chromium.org/2674593003/diff/40001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2674593003/diff/40001/src/objects.h#newcode8744
src/objects.h:8744: SharedFunctionInfo* info() const { return info_; }
On 2017/02/02 13:42:08, Michael Starzinger wrote:
> nit: s/info/shared/

Done.

https://codereview.chromium.org/2674593003/diff/40001/src/objects.h#newcode8747
src/objects.h:8747: bool has_info() const { return info_ != nullptr; }
On 2017/02/02 13:42:08, Michael Starzinger wrote:
> nit: s/info/shared/

Done.

https://codereview.chromium.org/2674593003/diff/40001/src/objects.h#newcode8751
src/objects.h:8751: SharedFunctionInfo* info_;
On 2017/02/02 13:42:07, Michael Starzinger wrote:
> nit: s/info/shared/

Done.

https://codereview.chromium.org/2674593003/diff/40001/test/cctest/test-serial...
File test/cctest/test-serialize.cc (right):

https://codereview.chromium.org/2674593003/diff/40001/test/cctest/test-serial...
test/cctest/test-serialize.cc:266: isolate->compilation_cache()->Clear();
Change to CollectAllAvailableGarbage().

[commit-bot: I haz the power, 2017-02-03 12:55:06]

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

[commit-bot: I haz the power, 2017-02-03 12:55:07]

Try jobs failed on following builders:
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/20403)
  v8_linux64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng_triggered...)

[mvstanton, 2017-02-06 09:42:47]

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

[commit-bot: I haz the power, 2017-02-06 09:42:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-06 10:12:05]

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

[commit-bot: I haz the power, 2017-02-06 10:12:06]

Dry run: This issue passed the CQ dry run.

[mvstanton, 2017-02-06 10:15:07]

The CQ bit was checked by mvstanton@chromium.org

[mvstanton, 2017-02-06 10:15:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, mstarzinger@chromium.org, yangguo@chromium.org
Link to the patchset: https://codereview.chromium.org/2674593003/#ps140001
(title: "REBASE+liveedit fix.")

[commit-bot: I haz the power, 2017-02-06 10:15:17]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-06 10:18:07]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1486376107594360,
"parent_rev": "f3aa32d971e6a5098abe4bed0ff5c0831bb76bfa", "commit_rev":
"aea3ce3df3dc8838940e5db74c9d459a3ba999e3"}

[commit-bot: I haz the power, 2017-02-06 10:18:11]

Description was changed from

==========
[TypeFeedbackVector] Root feedback vectors at function literal site.

TypeFeedbackVectors are strongly rooted by a closure. However, in modern
JavaScript closures are created and abandoned more freely. An important
closure may not be present in the root-set at time of garbage collection,
even though we've cached optimized code and use it regularly. For
example, consider leaf functions in an event dispatching system. They may
well be "hot," but tragically non-present when we collect the heap.

Until now, we've relied on a weak root to cache the feedback vector in
this case. Since there is no way to signal intent or relative importance,
this weak root is as susceptible to clearing as any other weak root at
garbage collection time.

Meanwhile, the feedback vector has become more important. All of our
ICs store their data there. Literal and regex boilerplates are stored there.
If we lose the vector, then we not only lose optimized code built from
it, we also lose the very feedback which allowed us to create that optimized
code. Therefore it's vital to express that dependency through the root
set.

This CL does this by creating a strong link to a feedback
vector at the instantiation site of the function closure.
This instantiation site is in the code and feedback vector
of the outer closure.

BUG=v8:5456
==========

to

==========
[TypeFeedbackVector] Root feedback vectors at function literal site.

TypeFeedbackVectors are strongly rooted by a closure. However, in modern
JavaScript closures are created and abandoned more freely. An important
closure may not be present in the root-set at time of garbage collection,
even though we've cached optimized code and use it regularly. For
example, consider leaf functions in an event dispatching system. They may
well be "hot," but tragically non-present when we collect the heap.

Until now, we've relied on a weak root to cache the feedback vector in
this case. Since there is no way to signal intent or relative importance,
this weak root is as susceptible to clearing as any other weak root at
garbage collection time.

Meanwhile, the feedback vector has become more important. All of our
ICs store their data there. Literal and regex boilerplates are stored there.
If we lose the vector, then we not only lose optimized code built from
it, we also lose the very feedback which allowed us to create that optimized
code. Therefore it's vital to express that dependency through the root
set.

This CL does this by creating a strong link to a feedback
vector at the instantiation site of the function closure.
This instantiation site is in the code and feedback vector
of the outer closure.

BUG=v8:5456

Review-Url: https://codereview.chromium.org/2674593003
Cr-Commit-Position: refs/heads/master@{#42953}
Committed:
https://chromium.googlesource.com/v8/v8/+/aea3ce3df3dc8838940e5db74c9d459a3ba...
==========

[commit-bot: I haz the power, 2017-02-06 10:18:13]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/v8/v8/+/aea3ce3df3dc8838940e5db74c9d459a3ba...