Remove infrastructure for experimental JS natives

Remove infrastructure for experimental JS natives

Now that no harmony-flagged features are implemented in experimental
JS, most of this is simply dead code.

As PostExperimentals() is no longer needed, I also removed the use of
Import() in the debug context, allowing the deletion of PostDebug()
along with PostExperimentals(); cleanup code is moved to the
end of PostNatives.

Also gets rid of some longer-dead code in prologue.js related to
TypedArrays, and some duplicate code for setting up SharedArrayBuffer
builtins.

Review-Url: https://codereview.chromium.org/2705293004
Cr-Commit-Position: refs/heads/master@{#43418}
Committed: https://chromium.googlesource.com/v8/v8/+/8cfe45b6f1880008d09d4310458b6e6fcb0522dd

[adamk, 2017-02-21 22:24:57]

Description was changed from

==========
Remove infrastructure for experimental JS natives

Now that no harmony-flagged features are implemented in experimental JS,
most of this is simply dead code.
==========

to

==========
Remove infrastructure for experimental JS natives

Now that no harmony-flagged features are implemented in experimental JS,
most of this is simply dead code.

Also gets rid of some longer-dead code in prologue.js related to
TypedArrays, and some duplicate code for setting up SharedArrayBuffer
builtins.
==========

[adamk, 2017-02-21 22:26:48]

adamk@chromium.org changed reviewers:
+ hpayer@chromium.org, yangguo@chromium.org

[adamk, 2017-02-21 22:26:49]

https://codereview.chromium.org/2705293004/diff/1/src/js/prologue.js
File src/js/prologue.js (left):

https://codereview.chromium.org/2705293004/diff/1/src/js/prologue.js#oldcode215
src/js/prologue.js:215: utils.PostDebug = UNDEFINED;
I don't know if it was important for these to be set to UNDEFINED. If so, I can
add code in bootstrapper.cc to clear them out on the utils object from there
(without calling into script).

https://codereview.chromium.org/2705293004/diff/1/src/js/prologue.js
File src/js/prologue.js (right):

https://codereview.chromium.org/2705293004/diff/1/src/js/prologue.js#newcode151
src/js/prologue.js:151: // Exposed only for tests.
Sadly there are two mjsunit tests which test these things directly. It might
make sense to do something different in a followup patch but I've left them here
for simplicity.

https://codereview.chromium.org/2705293004/diff/1/test/cctest/heap/test-space...
File test/cctest/heap/test-spaces.cc (right):

https://codereview.chromium.org/2705293004/diff/1/test/cctest/heap/test-space...
test/cctest/heap/test-spaces.cc:530: // requires no extra space.
So this is unfortunate, but the existing test wasn't testing what it thought it
was testing. Because the bootstrapper always ran PostExperimentals(), we already
had bumped the number of code pages from whatever was in the snapshot. If we'd
like this fact to be true, other fixes (not related to this change) will be
necessary.

[adamk, 2017-02-21 22:57:39]

adamk@chromium.org changed reviewers:
+ littledan@chromium.org

[adamk, 2017-02-21 23:03:35]

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

[commit-bot: I haz the power, 2017-02-21 23:03:41]

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

[Dan Ehrenberg, 2017-02-21 23:06:04]

lgtm

From what I can tell, great job removing that code; I was in the middle of
writing a much less complete version of this patch, and this is much better.

Note that this will make it harder to revert an earlier patch of mine which
seems to be causing regressions in a couple benchmarks:

https://bugs.chromium.org/p/chromium/issues/detail?id=692988
https://bugs.chromium.org/p/chromium/issues/detail?id=694568

I can't see V8 performance graphs anymore, so I can't tell if anything improved
due to the previous patch, and I don't know what benchmark the second one is
talking about, but I've been able to reproduce locally the speedup on startup
from the previous patch, and the slowdown on Emscripten Fannkuch. From running
your patch locally, I can see that it improves startup performance further
(though not as much as the previous patch), but it seems like we still have a
regression compared on Emscripten Fannkuch vs the state of the world vs the
first patch.

I don't know what to make of the situation. I haven't figured out why Fannkuch
regressed. If other benchmarks improved by the same patch, then maybe we should
accept this regression and continue with this patch.

[commit-bot: I haz the power, 2017-02-21 23:32:11]

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

[commit-bot: I haz the power, 2017-02-21 23:32:11]

Dry run: This issue passed the CQ dry run.

[adamk, 2017-02-22 01:32:17]

On 2017/02/21 23:06:04, Dan Ehrenberg wrote:
> lgtm
> 
> From what I can tell, great job removing that code; I was in the middle of
> writing a much less complete version of this patch, and this is much better.

Thanks, sorry if I stepped on your toes. I checked your pending CLs and didn't
see anything, but I should have pinged before putting this together (I mostly
did it on a whim, after getting tired of seeing PostExperimentals getting
compiled when running trivial test cases...).

> Note that this will make it harder to revert an earlier patch of mine which
> seems to be causing regressions in a couple benchmarks:
> 
> https://bugs.chromium.org/p/chromium/issues/detail?id=692988
> https://bugs.chromium.org/p/chromium/issues/detail?id=694568
> 
> I can't see V8 performance graphs anymore, so I can't tell if anything
improved
> due to the previous patch, and I don't know what benchmark the second one is
> talking about, but I've been able to reproduce locally the speedup on startup
> from the previous patch, and the slowdown on Emscripten Fannkuch. From running
> your patch locally, I can see that it improves startup performance further
> (though not as much as the previous patch), but it seems like we still have a
> regression compared on Emscripten Fannkuch vs the state of the world vs the
> first patch.
> 
> I don't know what to make of the situation. I haven't figured out why Fannkuch
> regressed. If other benchmarks improved by the same patch, then maybe we
should
> accept this regression and continue with this patch.

Sorry about the "externals can't see perf graphs" problem.

From a quick glance at the graphs I see, this all looks like step-function
variation likely unrelated to the underlying change. Given the concrete benefits
of this change, per Yang, I don't think it's likely we'd revert anyway. Curious
what Yang's thoughts are, too; he might have a guess as to what we might have
tickled that'd affect these particular tests.

[Yang, 2017-02-22 09:38:14]

Awesome. LGTM with comments.

https://codereview.chromium.org/2705293004/diff/1/src/js/prologue.js
File src/js/prologue.js (left):

https://codereview.chromium.org/2705293004/diff/1/src/js/prologue.js#oldcode215
src/js/prologue.js:215: utils.PostDebug = UNDEFINED;
On 2017/02/21 22:26:49, adamk wrote:
> I don't know if it was important for these to be set to UNDEFINED. If so, I
can
> add code in bootstrapper.cc to clear them out on the utils object from there
> (without calling into script).

It should suffice to just move these over to PostNatives. We should also remove
PostDebug once possible.

You can also remove typed_array_setup and its use sites. It's no longer needed.

https://codereview.chromium.org/2705293004/diff/1/src/js/prologue.js
File src/js/prologue.js (right):

https://codereview.chromium.org/2705293004/diff/1/src/js/prologue.js#newcode150
src/js/prologue.js:150: "SetValues",
These functions could be pulled directly from Map.prototype and Set.prototype.
In the debug context, they are unpolluted.

https://codereview.chromium.org/2705293004/diff/1/test/cctest/heap/test-space...
File test/cctest/heap/test-spaces.cc (right):

https://codereview.chromium.org/2705293004/diff/1/test/cctest/heap/test-space...
test/cctest/heap/test-spaces.cc:530: // requires no extra space.
On 2017/02/21 22:26:49, adamk wrote:
> So this is unfortunate, but the existing test wasn't testing what it thought
it
> was testing. Because the bootstrapper always ran PostExperimentals(), we
already
> had bumped the number of code pages from whatever was in the snapshot. If we'd
> like this fact to be true, other fixes (not related to this change) will be
> necessary.

Nice catch. I think the proposed change here is fine. We have already moved away
from what this test was supposed to test. Drive down the memory lane: we used to
reserve slightly more space than necessary when creating an isolate so that
running very small pieces of JS after start up will not cause allocation of a
new page immediately. We have since moved away from this, and the impact of
allocating a page is smaller since the page size is now 500kB instead of 1MB

[adamk, 2017-02-22 20:24:18]

Description was changed from

==========
Remove infrastructure for experimental JS natives

Now that no harmony-flagged features are implemented in experimental JS,
most of this is simply dead code.

Also gets rid of some longer-dead code in prologue.js related to
TypedArrays, and some duplicate code for setting up SharedArrayBuffer
builtins.
==========

to

==========
Remove infrastructure for experimental JS natives

Now that no harmony-flagged features are implemented in experimental JS, most of
this is simply dead code.

As PostExperimentals() is no longer needed, I also removed the use of
Import() in the debug context, allowing the deletion of PostDebug()
along with PostExperimentals(); cleanup code is moved to the
end of PostNatives. A few tests still depend on ImportNow(),
so cleanup is not fully complete.

Also gets rid of some longer-dead code in prologue.js related to
TypedArrays, and some duplicate code for setting up SharedArrayBuffer
builtins.
==========

[adamk, 2017-02-22 20:25:17]

https://codereview.chromium.org/2705293004/diff/1/src/js/prologue.js
File src/js/prologue.js (left):

https://codereview.chromium.org/2705293004/diff/1/src/js/prologue.js#oldcode215
src/js/prologue.js:215: utils.PostDebug = UNDEFINED;
On 2017/02/22 09:38:14, Yang wrote:
> On 2017/02/21 22:26:49, adamk wrote:
> > I don't know if it was important for these to be set to UNDEFINED. If so, I
> can
> > add code in bootstrapper.cc to clear them out on the utils object from there
> > (without calling into script).
> 
> It should suffice to just move these over to PostNatives. We should also
remove
> PostDebug once possible.

I can't do this in PostNatives since then the debugger context can't be set up
properly. Unless you're suggesting I just remove PostDebug as part of this
change. The latter is what I've done for now, let me know if this is what you
meant.

> You can also remove typed_array_setup and its use sites. It's no longer
needed.

I think I already did (see the top of this file). Let me know if I'm missing
something.

https://codereview.chromium.org/2705293004/diff/1/src/js/prologue.js
File src/js/prologue.js (right):

https://codereview.chromium.org/2705293004/diff/1/src/js/prologue.js#newcode150
src/js/prologue.js:150: "SetValues",
On 2017/02/22 09:38:14, Yang wrote:
> These functions could be pulled directly from Map.prototype and Set.prototype.
> In the debug context, they are unpolluted.

Getting ahold of the .next methods isn't possible without allocating a Map and a
Set. That's what I've done now, but it looks a little funny.

https://codereview.chromium.org/2705293004/diff/1/test/cctest/heap/test-space...
File test/cctest/heap/test-spaces.cc (right):

https://codereview.chromium.org/2705293004/diff/1/test/cctest/heap/test-space...
test/cctest/heap/test-spaces.cc:530: // requires no extra space.
On 2017/02/22 09:38:14, Yang wrote:
> On 2017/02/21 22:26:49, adamk wrote:
> > So this is unfortunate, but the existing test wasn't testing what it thought
> it
> > was testing. Because the bootstrapper always ran PostExperimentals(), we
> already
> > had bumped the number of code pages from whatever was in the snapshot. If
we'd
> > like this fact to be true, other fixes (not related to this change) will be
> > necessary.
> 
> Nice catch. I think the proposed change here is fine. We have already moved
away
> from what this test was supposed to test. Drive down the memory lane: we used
to
> reserve slightly more space than necessary when creating an isolate so that
> running very small pieces of JS after start up will not cause allocation of a
> new page immediately. We have since moved away from this, and the impact of
> allocating a page is smaller since the page size is now 500kB instead of 1MB

Okay. Hannes, need your LGTM for heap as well.

[adamk, 2017-02-22 20:28:43]

Description was changed from

==========
Remove infrastructure for experimental JS natives

Now that no harmony-flagged features are implemented in experimental JS, most of
this is simply dead code.

As PostExperimentals() is no longer needed, I also removed the use of
Import() in the debug context, allowing the deletion of PostDebug()
along with PostExperimentals(); cleanup code is moved to the
end of PostNatives. A few tests still depend on ImportNow(),
so cleanup is not fully complete.

Also gets rid of some longer-dead code in prologue.js related to
TypedArrays, and some duplicate code for setting up SharedArrayBuffer
builtins.
==========

to

==========
Remove infrastructure for experimental JS natives

Now that no harmony-flagged features are implemented in experimental
JS, most of this is simply dead code.

As PostExperimentals() is no longer needed, I also removed the use of
Import() in the debug context, allowing the deletion of PostDebug()
along with PostExperimentals(); cleanup code is moved to the
end of PostNatives. A few tests still depend on ImportNow(),
so cleanup is not fully complete.

Also gets rid of some longer-dead code in prologue.js related to
TypedArrays, and some duplicate code for setting up SharedArrayBuffer
builtins.
==========

[adamk, 2017-02-22 20:28:58]

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

[commit-bot: I haz the power, 2017-02-22 20:29: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, 2017-02-22 21:32:11]

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

[commit-bot: I haz the power, 2017-02-22 21:32:13]

Dry run: Try jobs failed on following builders:
  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_arm_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng_trigger...)

[adamk, 2017-02-22 21:35:23]

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

[commit-bot: I haz the power, 2017-02-22 21:35:34]

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-22 22:10:45]

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

[commit-bot: I haz the power, 2017-02-22 22:10:46]

Dry run: This issue passed the CQ dry run.

[adamk, 2017-02-22 22:53:31]

I've got two CLs out that remove the mjsunit test dependencies (by deleting the
tests);

https://codereview.chromium.org/2707133005/
https://codereview.chromium.org/2711833002/

Each CL explains why I think it's OK to lose these tests.

[adamk, 2017-02-23 19:06:07]

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

[commit-bot: I haz the power, 2017-02-23 19:06:17]

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

[adamk, 2017-02-23 19:06:19]

Description was changed from

==========
Remove infrastructure for experimental JS natives

Now that no harmony-flagged features are implemented in experimental
JS, most of this is simply dead code.

As PostExperimentals() is no longer needed, I also removed the use of
Import() in the debug context, allowing the deletion of PostDebug()
along with PostExperimentals(); cleanup code is moved to the
end of PostNatives. A few tests still depend on ImportNow(),
so cleanup is not fully complete.

Also gets rid of some longer-dead code in prologue.js related to
TypedArrays, and some duplicate code for setting up SharedArrayBuffer
builtins.
==========

to

==========
Remove infrastructure for experimental JS natives

Now that no harmony-flagged features are implemented in experimental
JS, most of this is simply dead code.

As PostExperimentals() is no longer needed, I also removed the use of
Import() in the debug context, allowing the deletion of PostDebug()
along with PostExperimentals(); cleanup code is moved to the
end of PostNatives. A few tests still depend on ImportNow(),
so cleanup is not fully complete.

Also gets rid of some longer-dead code in prologue.js related to
TypedArrays, and some duplicate code for setting up SharedArrayBuffer
builtins.

BUG=
==========

[adamk, 2017-02-23 19:07:03]

Description was changed from

==========
Remove infrastructure for experimental JS natives

Now that no harmony-flagged features are implemented in experimental
JS, most of this is simply dead code.

As PostExperimentals() is no longer needed, I also removed the use of
Import() in the debug context, allowing the deletion of PostDebug()
along with PostExperimentals(); cleanup code is moved to the
end of PostNatives. A few tests still depend on ImportNow(),
so cleanup is not fully complete.

Also gets rid of some longer-dead code in prologue.js related to
TypedArrays, and some duplicate code for setting up SharedArrayBuffer
builtins.

BUG=
==========

to

==========
Remove infrastructure for experimental JS natives

Now that no harmony-flagged features are implemented in experimental
JS, most of this is simply dead code.

As PostExperimentals() is no longer needed, I also removed the use of
Import() in the debug context, allowing the deletion of PostDebug()
along with PostExperimentals(); cleanup code is moved to the
end of PostNatives.

Also gets rid of some longer-dead code in prologue.js related to
TypedArrays, and some duplicate code for setting up SharedArrayBuffer
builtins.
==========

[adamk, 2017-02-23 19:08:13]

I've merged in the removal of the test dependencies on the import/export system,
so this is even cleaner now. PTAL:

yangguo for debug/
hpayer for heap/

[commit-bot: I haz the power, 2017-02-23 19:39:51]

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

[commit-bot: I haz the power, 2017-02-23 19:39:52]

Dry run: This issue passed the CQ dry run.

[Yang, 2017-02-24 07:37:37]

On 2017/02/23 19:08:13, adamk wrote:
> I've merged in the removal of the test dependencies on the import/export
system,
> so this is even cleaner now. PTAL:
> 
> yangguo for debug/
> hpayer for heap/

lgtm.

[Hannes Payer (out of office), 2017-02-24 13:03:02]

heap lgtm

[adamk, 2017-02-24 17:39:19]

The CQ bit was checked by adamk@chromium.org

[adamk, 2017-02-24 17:39:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
littledan@chromium.org
Link to the patchset: https://codereview.chromium.org/2705293004/#ps60001
(title: "Rebased")

[commit-bot: I haz the power, 2017-02-24 17:39:31]

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-24 17:43:29]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487957959560150,
"parent_rev": "0ba513f05696dbf241959b3e0de6a3f8ef2eeab3", "commit_rev":
"8cfe45b6f1880008d09d4310458b6e6fcb0522dd"}

[commit-bot: I haz the power, 2017-02-24 17:43:35]

Description was changed from

==========
Remove infrastructure for experimental JS natives

Now that no harmony-flagged features are implemented in experimental
JS, most of this is simply dead code.

As PostExperimentals() is no longer needed, I also removed the use of
Import() in the debug context, allowing the deletion of PostDebug()
along with PostExperimentals(); cleanup code is moved to the
end of PostNatives.

Also gets rid of some longer-dead code in prologue.js related to
TypedArrays, and some duplicate code for setting up SharedArrayBuffer
builtins.
==========

to

==========
Remove infrastructure for experimental JS natives

Now that no harmony-flagged features are implemented in experimental
JS, most of this is simply dead code.

As PostExperimentals() is no longer needed, I also removed the use of
Import() in the debug context, allowing the deletion of PostDebug()
along with PostExperimentals(); cleanup code is moved to the
end of PostNatives.

Also gets rid of some longer-dead code in prologue.js related to
TypedArrays, and some duplicate code for setting up SharedArrayBuffer
builtins.

Review-Url: https://codereview.chromium.org/2705293004
Cr-Commit-Position: refs/heads/master@{#43418}
Committed:
https://chromium.googlesource.com/v8/v8/+/8cfe45b6f1880008d09d4310458b6e6fcb0...
==========

[commit-bot: I haz the power, 2017-02-24 17:43:36]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/v8/v8/+/8cfe45b6f1880008d09d4310458b6e6fcb0...