Store SharedFunctionInfos of a Script in a FixedArray indexed by their ID

Store SharedFunctionInfos of a Script in a FixedArray indexed by their ID

Now that SharedFunctionInfos have a unique ID (and the IDs are dense),
we can use them as an index into an array, instead of using a
WeakFixedArray where we have to do a linear scan.

Hooking up liveedit is a bit more involved, see
https://docs.google.com/presentation/d/1FtNa3U7WsF5bPhY9uGoJG5Y9hnz5VBDabfOWpb4unWI/edit
for an overview

BUG=v8:5589
R=verwaest@chromium.org,jgruber@chromium.org

Committed: https://crrev.com/6595e7405769dc9d49e9568d61485efc6d468baf
Cr-Commit-Position: refs/heads/master@{#41600}

[jochen (gone - plz use gerrit), 2016-12-01 10:25:20]

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

[commit-bot: I haz the power, 2016-12-01 10:25:24]

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

[jochen (gone - plz use gerrit), 2016-12-01 10:30:39]

Description was changed from

==========
Store SharedFunctionInfos of a Script in a FixedArray indexed by their ID

Now that SharedFunctionInfos have a unique ID (and the IDs are dense),
we can use them as an index into an array, instead of using a
WeakFixedArray where we have to do a linear scan.

BUG=v8:5589
R=verwaest@chromium.org,jgruber@chromium.rg
==========

to

==========
Store SharedFunctionInfos of a Script in a FixedArray indexed by their ID

Now that SharedFunctionInfos have a unique ID (and the IDs are dense),
we can use them as an index into an array, instead of using a
WeakFixedArray where we have to do a linear scan.

BUG=v8:5589
R=verwaest@chromium.org,jgruber@chromium.org
==========

[jochen (gone - plz use gerrit), 2016-12-01 10:30:39]

jochen@chromium.org changed reviewers:
+ jgruber@chromium.org
- jgruber@chromium.rg

[jochen (gone - plz use gerrit), 2016-12-01 10:30:56]

ptal

Jakob: liveedit specific changes
Toon: rest

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

https://codereview.chromium.org/2547483002/diff/1/src/debug/liveedit.js#newco...
src/debug/liveedit.js:86: MathMax.apply(null, new_compile_info.map(i =>
i.function_literal_id));
https://bugs.chromium.org/p/v8/issues/detail?id=5703 for why I'm not using the
spread operator here

[jgruber, 2016-12-01 11:00:15]

lgtm for liveedit

[commit-bot: I haz the power, 2016-12-01 11:12:58]

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

[commit-bot: I haz the power, 2016-12-01 11:12:59]

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

[jochen (gone - plz use gerrit), 2016-12-01 15:12:28]

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

[commit-bot: I haz the power, 2016-12-01 15:12:32]

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, 2016-12-01 15:28:41]

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

[commit-bot: I haz the power, 2016-12-01 15:28:42]

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

[jochen (gone - plz use gerrit), 2016-12-05 14:01:53]

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

[commit-bot: I haz the power, 2016-12-05 14:01:58]

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, 2016-12-05 14:34:07]

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

[commit-bot: I haz the power, 2016-12-05 14:34:08]

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

[jochen (gone - plz use gerrit), 2016-12-05 16:25:41]

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

[commit-bot: I haz the power, 2016-12-05 16:25:46]

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, 2016-12-05 16:42:30]

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

[commit-bot: I haz the power, 2016-12-05 16:42:30]

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_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)

[jochen (gone - plz use gerrit), 2016-12-05 20:27:38]

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

[commit-bot: I haz the power, 2016-12-05 20:27:49]

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, 2016-12-05 21:00:03]

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

[commit-bot: I haz the power, 2016-12-05 21:00:04]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2016-12-06 10:19:04]

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

[commit-bot: I haz the power, 2016-12-06 10:19:15]

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, 2016-12-06 10:46:58]

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

[commit-bot: I haz the power, 2016-12-06 10:46:59]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2016-12-06 14:39:17]

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

[commit-bot: I haz the power, 2016-12-06 14:39:27]

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, 2016-12-06 15:08:20]

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

[commit-bot: I haz the power, 2016-12-06 15:08:21]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2016-12-06 20:17:45]

Description was changed from

==========
Store SharedFunctionInfos of a Script in a FixedArray indexed by their ID

Now that SharedFunctionInfos have a unique ID (and the IDs are dense),
we can use them as an index into an array, instead of using a
WeakFixedArray where we have to do a linear scan.

BUG=v8:5589
R=verwaest@chromium.org,jgruber@chromium.org
==========

to

==========
Store SharedFunctionInfos of a Script in a FixedArray indexed by their ID

Now that SharedFunctionInfos have a unique ID (and the IDs are dense),
we can use them as an index into an array, instead of using a
WeakFixedArray where we have to do a linear scan.

Hooking up liveedit is a bit more involved, see
https://docs.google.com/presentation/d/1FtNa3U7WsF5bPhY9uGoJG5Y9hnz5VBDabfOWp...
for an overview

BUG=v8:5589
R=verwaest@chromium.org,jgruber@chromium.org
==========

[jochen (gone - plz use gerrit), 2016-12-06 20:18:21]

ptal now that the bots are green. I tried to visualize the changes to liveedit
and put them in a small presentation here:
https://docs.google.com/presentation/d/1FtNa3U7WsF5bPhY9uGoJG5Y9hnz5VBDabfOWp...

[Toon Verwaest, 2016-12-06 21:10:14]

Mostly minor comments.
Jakob should have a look again as well for the LiveEdit part though (or Yang).

https://codereview.chromium.org/2547483002/diff/110001/src/debug/debug.cc
File src/debug/debug.cc (right):

https://codereview.chromium.org/2547483002/diff/110001/src/debug/debug.cc#new...
src/debug/debug.cc:1340: if (raw->IsSmi() || WeakCell::cast(raw)->cleared())
continue;
I guess we should just install a cleared weak-cell as default value; but it
probably doesn't matter much; and could even be worse due to gc.

https://codereview.chromium.org/2547483002/diff/110001/src/debug/debug.cc#new...
src/debug/debug.cc:1342: SharedFunctionInfo::cast(WeakCell::cast(raw)->value());
This actually makes me wonder if we shouldn't just have something like
SharedFunctionInfo* info = WeakCell::cast(raw)->value<SharedFunctionInfo>();

That would save us 6 characters on basically each weak-cell read :)

https://codereview.chromium.org/2547483002/diff/110001/src/debug/debug.cc#new...
src/debug/debug.cc:1464: for (int index = 0; index < infos->length(); ++index) {
Given that we iterate at least 3 times, does it make sense to have an iterator
that encapsulates this?

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

https://codereview.chromium.org/2547483002/diff/110001/src/debug/liveedit.cc#...
src/debug/liveedit.cc:1078: #if 0
left-over code?

https://codereview.chromium.org/2547483002/diff/110001/src/debug/liveedit.js
File src/debug/liveedit.js (right):

https://codereview.chromium.org/2547483002/diff/110001/src/debug/liveedit.js#...
src/debug/liveedit.js:86: MathMax.apply(null, new_compile_info.map(i =>
i.function_literal_id));
What about just +new_compile_info.reduce((a,b)=>a>b?a:b)) with
i.valueOf(){return function_literal_id}? At the least that doesn't make the
stack grow like crazy :)

https://codereview.chromium.org/2547483002/diff/110001/src/debug/liveedit.js#...
src/debug/liveedit.js:235: // Link to an actual script all the functions that we
are going to use.
Link all the function we're going to use to an actual script?

https://codereview.chromium.org/2547483002/diff/110001/src/factory.h
File src/factory.h (right):

https://codereview.chromium.org/2547483002/diff/110001/src/factory.h#newcode49
src/factory.h:49: Handle<FixedArray> NewFixedArrayWithSmis(
This doesn't seem like a very generic function; do we really even need to have
an array with smis rather than just undefined? Checking for undefined is fast...

https://codereview.chromium.org/2547483002/diff/110001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2547483002/diff/110001/src/objects.cc#newcode...
src/objects.cc:13646: if (fun->function_literal_id() ==
FunctionLiteral::kIdTypeInvalid ||
Mmh. DCHECK_NE(id, Invalid); DCHECK_LT(id, length); ?

https://codereview.chromium.org/2547483002/diff/110001/src/objects.cc#newcode...
src/objects.cc:13697: DCHECK(shared->function_literal_id() !=
FunctionLiteral::kIdTypeInvalid);
DCHECK_NE

https://codereview.chromium.org/2547483002/diff/110001/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/2547483002/diff/110001/src/parsing/parser.cc#...
src/parsing/parser.cc:823:
info->set_max_function_literal_id(GetLastFunctionLiteralId());
Mmh. ParseInfo isn't exactly supposed to be a communication channel outerwards
right? What about putting it on the ScriptScope or so after ParseProgram? Isn't
that the only place where we need it? We probably need to make ScriptScope its
own subclass of Scope in that case. (It's not currently if I'm not mistaken)

https://codereview.chromium.org/2547483002/diff/110001/src/runtime/runtime-li...
File src/runtime/runtime-liveedit.cc (right):

https://codereview.chromium.org/2547483002/diff/110001/src/runtime/runtime-li...
src/runtime/runtime-liveedit.cc:108: DCHECK(args.length() == 2);
DCHECK_EQ

[jgruber, 2016-12-07 08:31:27]

Still lgtm, with the caveat that I don't know the liveedit code well.

https://codereview.chromium.org/2547483002/diff/110001/src/runtime/runtime-li...
File src/runtime/runtime-liveedit.cc (right):

https://codereview.chromium.org/2547483002/diff/110001/src/runtime/runtime-li...
src/runtime/runtime-liveedit.cc:110: CONVERT_NUMBER_CHECKED(int,
max_function_literal_id, Int32, args[1]);
Nit: CONVERT_INT32_ARG_CHECKED here and below.

[jochen (gone - plz use gerrit), 2016-12-07 15:57:01]

https://codereview.chromium.org/2547483002/diff/110001/src/debug/debug.cc
File src/debug/debug.cc (right):

https://codereview.chromium.org/2547483002/diff/110001/src/debug/debug.cc#new...
src/debug/debug.cc:1340: if (raw->IsSmi() || WeakCell::cast(raw)->cleared())
continue;
On 2016/12/06 at 21:10:14, Toon Verwaest wrote:
> I guess we should just install a cleared weak-cell as default value; but it
probably doesn't matter much; and could even be worse due to gc.

I considered that, but it would create a lot of overhead when allocating the
arrays (and make them considerably larger)

https://codereview.chromium.org/2547483002/diff/110001/src/debug/debug.cc#new...
src/debug/debug.cc:1342: SharedFunctionInfo::cast(WeakCell::cast(raw)->value());
On 2016/12/06 at 21:10:14, Toon Verwaest wrote:
> This actually makes me wonder if we shouldn't just have something like
SharedFunctionInfo* info = WeakCell::cast(raw)->value<SharedFunctionInfo>();
> 
> That would save us 6 characters on basically each weak-cell read :)

if we do that, I think it should be a separate clean-up replacing all existing
callsites of T::cast(WeakCell::cast(..)->value())

https://codereview.chromium.org/2547483002/diff/110001/src/debug/debug.cc#new...
src/debug/debug.cc:1464: for (int index = 0; index < infos->length(); ++index) {
On 2016/12/06 at 21:10:14, Toon Verwaest wrote:
> Given that we iterate at least 3 times, does it make sense to have an iterator
that encapsulates this?

done.

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

https://codereview.chromium.org/2547483002/diff/110001/src/debug/liveedit.cc#...
src/debug/liveedit.cc:1078: #if 0
On 2016/12/06 at 21:10:14, Toon Verwaest wrote:
> left-over code?

yes, removed

https://codereview.chromium.org/2547483002/diff/110001/src/debug/liveedit.js
File src/debug/liveedit.js (right):

https://codereview.chromium.org/2547483002/diff/110001/src/debug/liveedit.js#...
src/debug/liveedit.js:86: MathMax.apply(null, new_compile_info.map(i =>
i.function_literal_id));
On 2016/12/06 at 21:10:14, Toon Verwaest wrote:
> What about just +new_compile_info.reduce((a,b)=>a>b?a:b)) with
i.valueOf(){return function_literal_id}? At the least that doesn't make the
stack grow like crazy :)

why does this make the stack grow like crazy?

https://codereview.chromium.org/2547483002/diff/110001/src/debug/liveedit.js#...
src/debug/liveedit.js:235: // Link to an actual script all the functions that we
are going to use.
On 2016/12/06 at 21:10:14, Toon Verwaest wrote:
> Link all the function we're going to use to an actual script?

done

https://codereview.chromium.org/2547483002/diff/110001/src/factory.h
File src/factory.h (right):

https://codereview.chromium.org/2547483002/diff/110001/src/factory.h#newcode49
src/factory.h:49: Handle<FixedArray> NewFixedArrayWithSmis(
On 2016/12/06 at 21:10:14, Toon Verwaest wrote:
> This doesn't seem like a very generic function; do we really even need to have
an array with smis rather than just undefined? Checking for undefined is fast...

done

https://codereview.chromium.org/2547483002/diff/110001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2547483002/diff/110001/src/objects.cc#newcode...
src/objects.cc:13646: if (fun->function_literal_id() ==
FunctionLiteral::kIdTypeInvalid ||
On 2016/12/06 at 21:10:14, Toon Verwaest wrote:
> Mmh. DCHECK_NE(id, Invalid); DCHECK_LT(id, length); ?

done

https://codereview.chromium.org/2547483002/diff/110001/src/objects.cc#newcode...
src/objects.cc:13697: DCHECK(shared->function_literal_id() !=
FunctionLiteral::kIdTypeInvalid);
On 2016/12/06 at 21:10:14, Toon Verwaest wrote:
> DCHECK_NE

done

https://codereview.chromium.org/2547483002/diff/110001/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/2547483002/diff/110001/src/parsing/parser.cc#...
src/parsing/parser.cc:823:
info->set_max_function_literal_id(GetLastFunctionLiteralId());
On 2016/12/06 at 21:10:14, Toon Verwaest wrote:
> Mmh. ParseInfo isn't exactly supposed to be a communication channel outerwards
right? What about putting it on the ScriptScope or so after ParseProgram? Isn't
that the only place where we need it? We probably need to make ScriptScope its
own subclass of Scope in that case. (It's not currently if I'm not mistaken)

it's also what communicates the parsing result to the outside (and the language
mode)

https://codereview.chromium.org/2547483002/diff/110001/src/runtime/runtime-li...
File src/runtime/runtime-liveedit.cc (right):

https://codereview.chromium.org/2547483002/diff/110001/src/runtime/runtime-li...
src/runtime/runtime-liveedit.cc:108: DCHECK(args.length() == 2);
On 2016/12/06 at 21:10:14, Toon Verwaest wrote:
> DCHECK_EQ

done

https://codereview.chromium.org/2547483002/diff/110001/src/runtime/runtime-li...
src/runtime/runtime-liveedit.cc:110: CONVERT_NUMBER_CHECKED(int,
max_function_literal_id, Int32, args[1]);
On 2016/12/07 at 08:31:27, jgruber wrote:
> Nit: CONVERT_INT32_ARG_CHECKED here and below.

done

[jochen (gone - plz use gerrit), 2016-12-07 15:57:09]

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

[commit-bot: I haz the power, 2016-12-07 15:57:18]

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, 2016-12-07 15:58:47]

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

[commit-bot: I haz the power, 2016-12-07 15:58:49]

Dry run: Try jobs failed on following builders:
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/17464)
  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_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/17523)
  v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  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/17434)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/30173)
  v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)
  v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/19166)

[jochen (gone - plz use gerrit), 2016-12-07 16:04:37]

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

[commit-bot: I haz the power, 2016-12-07 16:04:42]

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, 2016-12-07 16:06:10]

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

[commit-bot: I haz the power, 2016-12-07 16:06:11]

Dry run: Try jobs failed on following builders:
  v8_android_arm_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...)
  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_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_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  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_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/17435)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/30174)
  v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)

[jochen (gone - plz use gerrit), 2016-12-07 16:22:52]

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

[commit-bot: I haz the power, 2016-12-07 16:23:01]

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, 2016-12-07 16:52:08]

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

[commit-bot: I haz the power, 2016-12-07 16:52:10]

Dry run: This issue passed the CQ dry run.

[Toon Verwaest, 2016-12-08 07:31:31]

https://codereview.chromium.org/2547483002/diff/110001/src/debug/debug.cc
File src/debug/debug.cc (right):

https://codereview.chromium.org/2547483002/diff/110001/src/debug/debug.cc#new...
src/debug/debug.cc:1340: if (raw->IsSmi() || WeakCell::cast(raw)->cleared())
continue;
On 2016/12/07 15:57:00, jochen wrote:
> On 2016/12/06 at 21:10:14, Toon Verwaest wrote:
> > I guess we should just install a cleared weak-cell as default value; but it
> probably doesn't matter much; and could even be worse due to gc.
> 
> I considered that, but it would create a lot of overhead when allocating the
> arrays (and make them considerably larger)

Why? I presume we have a single preallocated cleared weak-cell somewhere? And if
we don't, we could create one?

https://codereview.chromium.org/2547483002/diff/110001/src/debug/debug.cc#new...
src/debug/debug.cc:1342: SharedFunctionInfo::cast(WeakCell::cast(raw)->value());
On 2016/12/07 15:57:00, jochen wrote:
> On 2016/12/06 at 21:10:14, Toon Verwaest wrote:
> > This actually makes me wonder if we shouldn't just have something like
> SharedFunctionInfo* info = WeakCell::cast(raw)->value<SharedFunctionInfo>();
> > 
> > That would save us 6 characters on basically each weak-cell read :)
> 
> if we do that, I think it should be a separate clean-up replacing all existing
> callsites of T::cast(WeakCell::cast(..)->value())

Makes sense

https://codereview.chromium.org/2547483002/diff/110001/src/debug/liveedit.js
File src/debug/liveedit.js (right):

https://codereview.chromium.org/2547483002/diff/110001/src/debug/liveedit.js#...
src/debug/liveedit.js:86: MathMax.apply(null, new_compile_info.map(i =>
i.function_literal_id));
On 2016/12/07 15:57:00, jochen wrote:
> On 2016/12/06 at 21:10:14, Toon Verwaest wrote:
> > What about just +new_compile_info.reduce((a,b)=>a>b?a:b)) with
> i.valueOf(){return function_literal_id}? At the least that doesn't make the
> stack grow like crazy :)
> 
> why does this make the stack grow like crazy?

.apply with a possibly huge array? Don't they all become stack-allocated
parameters?

https://codereview.chromium.org/2547483002/diff/110001/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/2547483002/diff/110001/src/parsing/parser.cc#...
src/parsing/parser.cc:823:
info->set_max_function_literal_id(GetLastFunctionLiteralId());
On 2016/12/07 15:57:00, jochen wrote:
> On 2016/12/06 at 21:10:14, Toon Verwaest wrote:
> > Mmh. ParseInfo isn't exactly supposed to be a communication channel
outerwards
> right? What about putting it on the ScriptScope or so after ParseProgram?
Isn't
> that the only place where we need it? We probably need to make ScriptScope its
> own subclass of Scope in that case. (It's not currently if I'm not mistaken)
> 
> it's also what communicates the parsing result to the outside (and the
language
> mode)

Oh ok.. buh. Follow-up cleanup then I guess...
Language mode should also just be read from the scope.

https://codereview.chromium.org/2547483002/diff/150001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2547483002/diff/150001/src/objects.cc#newcode...
src/objects.cc:13665: Isolate* isolate = script_->GetIsolate();
What about just stashing isolate and shared_function_infos on the iterator? That
way you can also use it in liveedit?

[Yang, 2016-12-08 11:51:54]

On 2016/12/08 07:31:31, Toon Verwaest wrote:
> https://codereview.chromium.org/2547483002/diff/110001/src/debug/debug.cc
> File src/debug/debug.cc (right):
> 
>
https://codereview.chromium.org/2547483002/diff/110001/src/debug/debug.cc#new...
> src/debug/debug.cc:1340: if (raw->IsSmi() || WeakCell::cast(raw)->cleared())
> continue;
> On 2016/12/07 15:57:00, jochen wrote:
> > On 2016/12/06 at 21:10:14, Toon Verwaest wrote:
> > > I guess we should just install a cleared weak-cell as default value; but
it
> > probably doesn't matter much; and could even be worse due to gc.
> > 
> > I considered that, but it would create a lot of overhead when allocating the
> > arrays (and make them considerably larger)
> 
> Why? I presume we have a single preallocated cleared weak-cell somewhere? And
if
> we don't, we could create one?
> 
>
https://codereview.chromium.org/2547483002/diff/110001/src/debug/debug.cc#new...
> src/debug/debug.cc:1342:
SharedFunctionInfo::cast(WeakCell::cast(raw)->value());
> On 2016/12/07 15:57:00, jochen wrote:
> > On 2016/12/06 at 21:10:14, Toon Verwaest wrote:
> > > This actually makes me wonder if we shouldn't just have something like
> > SharedFunctionInfo* info = WeakCell::cast(raw)->value<SharedFunctionInfo>();
> > > 
> > > That would save us 6 characters on basically each weak-cell read :)
> > 
> > if we do that, I think it should be a separate clean-up replacing all
existing
> > callsites of T::cast(WeakCell::cast(..)->value())
> 
> Makes sense
> 
> https://codereview.chromium.org/2547483002/diff/110001/src/debug/liveedit.js
> File src/debug/liveedit.js (right):
> 
>
https://codereview.chromium.org/2547483002/diff/110001/src/debug/liveedit.js#...
> src/debug/liveedit.js:86: MathMax.apply(null, new_compile_info.map(i =>
> i.function_literal_id));
> On 2016/12/07 15:57:00, jochen wrote:
> > On 2016/12/06 at 21:10:14, Toon Verwaest wrote:
> > > What about just +new_compile_info.reduce((a,b)=>a>b?a:b)) with
> > i.valueOf(){return function_literal_id}? At the least that doesn't make the
> > stack grow like crazy :)
> > 
> > why does this make the stack grow like crazy?
> 
> .apply with a possibly huge array? Don't they all become stack-allocated
> parameters?
> 
> https://codereview.chromium.org/2547483002/diff/110001/src/parsing/parser.cc
> File src/parsing/parser.cc (right):
> 
>
https://codereview.chromium.org/2547483002/diff/110001/src/parsing/parser.cc#...
> src/parsing/parser.cc:823:
> info->set_max_function_literal_id(GetLastFunctionLiteralId());
> On 2016/12/07 15:57:00, jochen wrote:
> > On 2016/12/06 at 21:10:14, Toon Verwaest wrote:
> > > Mmh. ParseInfo isn't exactly supposed to be a communication channel
> outerwards
> > right? What about putting it on the ScriptScope or so after ParseProgram?
> Isn't
> > that the only place where we need it? We probably need to make ScriptScope
its
> > own subclass of Scope in that case. (It's not currently if I'm not mistaken)
> > 
> > it's also what communicates the parsing result to the outside (and the
> language
> > mode)
> 
> Oh ok.. buh. Follow-up cleanup then I guess...
> Language mode should also just be read from the scope.
> 
> https://codereview.chromium.org/2547483002/diff/150001/src/objects.cc
> File src/objects.cc (right):
> 
>
https://codereview.chromium.org/2547483002/diff/150001/src/objects.cc#newcode...
> src/objects.cc:13665: Isolate* isolate = script_->GetIsolate();
> What about just stashing isolate and shared_function_infos on the iterator?
That
> way you can also use it in liveedit?

lgtm.

[jochen (gone - plz use gerrit), 2016-12-08 12:31:13]

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

[jochen (gone - plz use gerrit), 2016-12-08 12:31:14]

https://codereview.chromium.org/2547483002/diff/110001/src/debug/debug.cc
File src/debug/debug.cc (right):

https://codereview.chromium.org/2547483002/diff/110001/src/debug/debug.cc#new...
src/debug/debug.cc:1340: if (raw->IsSmi() || WeakCell::cast(raw)->cleared())
continue;
On 2016/12/08 at 07:31:30, Toon Verwaest wrote:
> On 2016/12/07 15:57:00, jochen wrote:
> > On 2016/12/06 at 21:10:14, Toon Verwaest wrote:
> > > I guess we should just install a cleared weak-cell as default value; but
it
> > probably doesn't matter much; and could even be worse due to gc.
> > 
> > I considered that, but it would create a lot of overhead when allocating the
> > arrays (and make them considerably larger)
> 
> Why? I presume we have a single preallocated cleared weak-cell somewhere? And
if we don't, we could create one?

no, we don't have that. The WeakCell currently has an invariant that a newly
created WeakCell is never "cleared"

https://codereview.chromium.org/2547483002/diff/110001/src/debug/liveedit.js
File src/debug/liveedit.js (right):

https://codereview.chromium.org/2547483002/diff/110001/src/debug/liveedit.js#...
src/debug/liveedit.js:86: MathMax.apply(null, new_compile_info.map(i =>
i.function_literal_id));
On 2016/12/08 at 07:31:30, Toon Verwaest wrote:
> On 2016/12/07 15:57:00, jochen wrote:
> > On 2016/12/06 at 21:10:14, Toon Verwaest wrote:
> > > What about just +new_compile_info.reduce((a,b)=>a>b?a:b)) with
> > i.valueOf(){return function_literal_id}? At the least that doesn't make the
> > stack grow like crazy :)
> > 
> > why does this make the stack grow like crazy?
> 
> .apply with a possibly huge array? Don't they all become stack-allocated
parameters?

k

https://codereview.chromium.org/2547483002/diff/150001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2547483002/diff/150001/src/objects.cc#newcode...
src/objects.cc:13665: Isolate* isolate = script_->GetIsolate();
On 2016/12/08 at 07:31:31, Toon Verwaest wrote:
> What about just stashing isolate and shared_function_infos on the iterator?
That way you can also use it in liveedit?

done

[jochen (gone - plz use gerrit), 2016-12-08 12:45:58]

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

[commit-bot: I haz the power, 2016-12-08 12:46:00]

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, 2016-12-08 13:04:55]

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

[commit-bot: I haz the power, 2016-12-08 13:04:56]

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

[Toon Verwaest, 2016-12-08 13:36:23]

lgtm

[jochen (gone - plz use gerrit), 2016-12-08 15:56:49]

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

[commit-bot: I haz the power, 2016-12-08 15:56:55]

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, 2016-12-08 16:25:15]

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

[commit-bot: I haz the power, 2016-12-08 16:25:16]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2016-12-08 17:04:18]

The CQ bit was checked by jochen@chromium.org

[jochen (gone - plz use gerrit), 2016-12-08 17:04:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
jgruber@chromium.org, yangguo@chromium.org, verwaest@chromium.org
Link to the patchset: https://codereview.chromium.org/2547483002/#ps190001
(title: "updates")

[commit-bot: I haz the power, 2016-12-08 17:04:30]

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, 2016-12-08 17:06:30]

CQ is committing da patch.

Bot data: {"patchset_id": 190001, "attempt_start_ts": 1481216658241760,
"parent_rev": "2d57dedacf6eb063287b93d364938cc8cb16c0fc", "commit_rev":
"9c78a20bc25615286f2bd3bf571d408977644f28"}

[commit-bot: I haz the power, 2016-12-08 17:06:39]

Committed patchset #11 (id:190001)

[commit-bot: I haz the power, 2016-12-08 17:07:18]

Description was changed from

==========
Store SharedFunctionInfos of a Script in a FixedArray indexed by their ID

Now that SharedFunctionInfos have a unique ID (and the IDs are dense),
we can use them as an index into an array, instead of using a
WeakFixedArray where we have to do a linear scan.

Hooking up liveedit is a bit more involved, see
https://docs.google.com/presentation/d/1FtNa3U7WsF5bPhY9uGoJG5Y9hnz5VBDabfOWp...
for an overview

BUG=v8:5589
R=verwaest@chromium.org,jgruber@chromium.org
==========

to

==========
Store SharedFunctionInfos of a Script in a FixedArray indexed by their ID

Now that SharedFunctionInfos have a unique ID (and the IDs are dense),
we can use them as an index into an array, instead of using a
WeakFixedArray where we have to do a linear scan.

Hooking up liveedit is a bit more involved, see
https://docs.google.com/presentation/d/1FtNa3U7WsF5bPhY9uGoJG5Y9hnz5VBDabfOWp...
for an overview

BUG=v8:5589
R=verwaest@chromium.org,jgruber@chromium.org

Committed: https://crrev.com/6595e7405769dc9d49e9568d61485efc6d468baf
Cr-Commit-Position: refs/heads/master@{#41600}
==========

[commit-bot: I haz the power, 2016-12-08 17:07:20]

Patchset 11 (id:??) landed as
https://crrev.com/6595e7405769dc9d49e9568d61485efc6d468baf
Cr-Commit-Position: refs/heads/master@{#41600}

[kozy, 2016-12-14 01:09:50]

kozyatinskiy@chromium.org changed reviewers:
+ kozyatinskiy@chromium.org

[kozy, 2016-12-14 01:09:51]

https://codereview.chromium.org/2547483002/diff/190001/src/debug/liveedit.js
File src/debug/liveedit.js (right):

https://codereview.chromium.org/2547483002/diff/190001/src/debug/liveedit.js#...
src/debug/liveedit.js:215: .corresponding_node.info.function_literal_id;
It could be no corresponding_node here as mentioned in comment:
  // For each old function (if it is not damaged) tries to find a corresponding
  // function in new script. Typically it should succeed (non-damaged functions
  // by definition may only have changes inside their bodies). However there are
  // reasons for correspondence not to be found; function with unmodified text
  // in new script may become enclosed into other function; the innocent change
  // inside function body may in fact be something like "} function B() {" that
  // splits a function into 2 functions.
Complicated repro case is described in crbug.com/673950

[kozy, 2016-12-14 01:10:37]

A revert of this CL (patchset #11 id:190001) has been created in
https://codereview.chromium.org/2578433002/ by kozyatinskiy@chromium.org.

The reason for reverting is: LiveEdit is broken in some cases.
crbug.com/673950.