[debugger] Refactor LiveEdit function info collection

[debugger] Refactor LiveEdit function info collection

This moves collection of function information from its previous spot in
the standard compiler pipeline (GetSharedFunctionInfo() and
CompileTopLevel()) to its new location in CompileForLiveEdit. Nesting
information is reconstructed by traversing the AST.

R=yangguo@chromium.org
BUG=

Committed: https://crrev.com/5bf3699221b8d38b4ea153def5d799e5725a8e7f
Cr-Commit-Position: refs/heads/master@{#36306}

[jgruber1, 2016-05-11 06:47:50]

The CQ bit was checked by jgruber@google.com

[commit-bot: I haz the power, 2016-05-11 06:48:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1971683002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1971683002/1

[commit-bot: I haz the power, 2016-05-11 06:48:01]

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

[commit-bot: I haz the power, 2016-05-11 06:48:02]

No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
Committers are members of the group "project-v8-committers".
Note that this has nothing to do with OWNERS files.

[Yang, 2016-05-11 07:29:10]

Looking good so far. Some comments.

https://codereview.chromium.org/1971683002/diff/1/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/1971683002/diff/1/src/compiler.cc#newcode1256
src/compiler.cc:1256: LiveEditFunctionTracker::Collect(script, &parse_info,
isolate);
The caller of CompileForLiveEdit (LiveEdit::GatherCompileInfo) sets the
FunctionInfoListener onto the isolate for LiveEditFunctionTracker to call into.
This should not be necessary. Instead, let's fold the functionality of
LiveEditFunctionTracker and FunctionInfoListener into LiveEditFunctionVisitor.
LiveEdit::GatherCompileInfo should just pass in an instance of
LiveEditFunctionVisitor into here.

LiveEditFunctionVisitor would have a Collect method, and hide all the visit
methods as private.

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

https://codereview.chromium.org/1971683002/diff/1/src/debug/liveedit.cc#newco...
src/debug/liveedit.cc:723: DCHECK(shared->IsSharedFunctionInfo());
We should not need this check. I don't even know why we had this in the first
place.

https://codereview.chromium.org/1971683002/diff/1/src/debug/liveedit.cc#newco...
src/debug/liveedit.cc:2035: // avoid duplicate code.
Yup. Let's use Script::FindSharedFunctionInfo instead. The (now wrong) assertion
in it should be moved into its other call site, GetSharedFunctionInfo.

https://codereview.chromium.org/1971683002/diff/1/src/debug/liveedit.cc#newco...
src/debug/liveedit.cc:2059: Handle<SharedFunctionInfo> info =
FindSharedFunctionInfo(node);
For the number of shared function infos in the script, the complexity of
FindSharedFunctionInfo is O(n), so for the whole script we are looking at
O(n^2). I'm not too worried about this right now, but we could add a TODO here
to optimize this if this becomes a problem. For example, for large scripts with
many entries in the list, sort upfront and do a binary search.

https://codereview.chromium.org/1971683002/diff/1/src/debug/liveedit.h
File src/debug/liveedit.h (right):

https://codereview.chromium.org/1971683002/diff/1/src/debug/liveedit.h#newcode41
src/debug/liveedit.h:41: class LiveEditFunctionTracker : AllStatic {
As previously described, we should merge the functionality of this and
FunctionInfoListener into LiveEditFunctionVisitor.

[jgruber1, 2016-05-12 11:16:49]

The CQ bit was checked by jgruber@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-05-12 11:16:53]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1971683002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1971683002/20001

[commit-bot: I haz the power, 2016-05-12 11:19:23]

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

[commit-bot: I haz the power, 2016-05-12 11:19:26]

Dry run: Try jobs failed on following builders:
  v8_android_arm_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...)
  v8_linux64_avx2_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux64_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/5663)
  v8_linux_arm64_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)
  v8_linux_arm_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)
  v8_linux_mips64el_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_linux_mipsel_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  v8_linux_nodcheck_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)

[jgruber1, 2016-05-12 12:30:25]

PTAL

https://codereview.chromium.org/1971683002/diff/1/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/1971683002/diff/1/src/compiler.cc#newcode1256
src/compiler.cc:1256: LiveEditFunctionTracker::Collect(script, &parse_info,
isolate);
On 2016/05/11 07:29:09, Yang wrote:
> The caller of CompileForLiveEdit (LiveEdit::GatherCompileInfo) sets the
> FunctionInfoListener onto the isolate for LiveEditFunctionTracker to call
into.
> This should not be necessary. Instead, let's fold the functionality of
> LiveEditFunctionTracker and FunctionInfoListener into LiveEditFunctionVisitor.
> LiveEdit::GatherCompileInfo should just pass in an instance of
> LiveEditFunctionVisitor into here.

Done.

> LiveEditFunctionVisitor would have a Collect method, and hide all the visit
> methods as private.

We can't hide visit methods since they're used by node::Accept().

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

https://codereview.chromium.org/1971683002/diff/1/src/debug/liveedit.cc#newco...
src/debug/liveedit.cc:723: DCHECK(shared->IsSharedFunctionInfo());
On 2016/05/11 07:29:09, Yang wrote:
> We should not need this check. I don't even know why we had this in the first
> place.

Done.

https://codereview.chromium.org/1971683002/diff/1/src/debug/liveedit.cc#newco...
src/debug/liveedit.cc:2035: // avoid duplicate code.
On 2016/05/11 07:29:09, Yang wrote:
> Yup. Let's use Script::FindSharedFunctionInfo instead. The (now wrong)
assertion
> in it should be moved into its other call site, GetSharedFunctionInfo.

Done.

https://codereview.chromium.org/1971683002/diff/1/src/debug/liveedit.cc#newco...
src/debug/liveedit.cc:2059: Handle<SharedFunctionInfo> info =
FindSharedFunctionInfo(node);
On 2016/05/11 07:29:09, Yang wrote:
> For the number of shared function infos in the script, the complexity of
> FindSharedFunctionInfo is O(n), so for the whole script we are looking at
> O(n^2). I'm not too worried about this right now, but we could add a TODO here
> to optimize this if this becomes a problem. For example, for large scripts
with
> many entries in the list, sort upfront and do a binary search.

Good point. The repeated linear search is pretty ugly. I've added a TODO for
now.

https://codereview.chromium.org/1971683002/diff/1/src/debug/liveedit.h
File src/debug/liveedit.h (right):

https://codereview.chromium.org/1971683002/diff/1/src/debug/liveedit.h#newcode41
src/debug/liveedit.h:41: class LiveEditFunctionTracker : AllStatic {
On 2016/05/11 07:29:09, Yang wrote:
> As previously described, we should merge the functionality of this and
> FunctionInfoListener into LiveEditFunctionVisitor.

Done.

[Yang, 2016-05-13 07:53:31]

https://codereview.chromium.org/1971683002/diff/20001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/1971683002/diff/20001/src/compiler.cc#newcode...
src/compiler.cc:1239: Handle<JSArray>* infos) {
Let's return a MaybeHandle<JSArray>. If compilation fails, we would just return
an empty Handle<JSArray>().

[jgruber1, 2016-05-13 08:31:49]

https://codereview.chromium.org/1971683002/diff/20001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/1971683002/diff/20001/src/compiler.cc#newcode...
src/compiler.cc:1239: Handle<JSArray>* infos) {
On 2016/05/13 07:53:30, Yang wrote:
> Let's return a MaybeHandle<JSArray>. If compilation fails, we would just
return
> an empty Handle<JSArray>().

Done.

[Yang, 2016-05-13 08:50:26]

LGTM.

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

https://codereview.chromium.org/1971683002/diff/40001/src/debug/liveedit.cc#n...
src/debug/liveedit.cc:738:
Compiler::CompileForLiveEdit(script).ToHandle(&infos);
Or we could just keep infos as a MaybeHandle.

[jgruber1, 2016-05-17 15:41:26]

The CQ bit was checked by jgruber@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-05-17 15:41:37]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1971683002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1971683002/40001

[commit-bot: I haz the power, 2016-05-17 16:14:00]

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

[commit-bot: I haz the power, 2016-05-17 16:14:01]

Dry run: This issue passed the CQ dry run.

[jgruber1, 2016-05-18 06:39:45]

The CQ bit was checked by jgruber@google.com

[jgruber1, 2016-05-18 06:39:45]

The patchset sent to the CQ was uploaded after l-g-t-m from yangguo@chromium.org
Link to the patchset: https://codereview.chromium.org/1971683002/#ps60001
(title: "Keep infos as MaybeHandle")

[commit-bot: I haz the power, 2016-05-18 06:39:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1971683002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1971683002/60001

[commit-bot: I haz the power, 2016-05-18 07:09:44]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-05-18 07:10:28]

Description was changed from

==========
[debugger] Refactor LiveEdit function info collection

This moves collection of function information from its previous spot in
the standard compiler pipeline (GetSharedFunctionInfo() and
CompileTopLevel()) to its new location in CompileForLiveEdit. Nesting
information is reconstructed by traversing the AST.

R=yangguo@chromium.org
BUG=
==========

to

==========
[debugger] Refactor LiveEdit function info collection

This moves collection of function information from its previous spot in
the standard compiler pipeline (GetSharedFunctionInfo() and
CompileTopLevel()) to its new location in CompileForLiveEdit. Nesting
information is reconstructed by traversing the AST.

R=yangguo@chromium.org
BUG=

Committed: https://crrev.com/5bf3699221b8d38b4ea153def5d799e5725a8e7f
Cr-Commit-Position: refs/heads/master@{#36306}
==========

[commit-bot: I haz the power, 2016-05-18 07:10:29]

Patchset 4 (id:??) landed as
https://crrev.com/5bf3699221b8d38b4ea153def5d799e5725a8e7f
Cr-Commit-Position: refs/heads/master@{#36306}