[wasm] Refactor code specialization / patching

[wasm] Refactor code specialization / patching

All patching logic is now bundled in one compilation unit.
The CodeSpecialization object is set up by all relocation and patching
that should be applied, and then be run on individual code objects or
the whole instance in one go. We hence only need to iterate all
relocation tables exactly once at instantiation.
Also, we do not patch contexts any more since we do not embed them in
generated code any more.

R=titzer@chromium.org
BUG=v8:5991
Review-Url: https://codereview.chromium.org/2696143006
Cr-Commit-Position: refs/heads/master@{#43324}
Committed: https://chromium.googlesource.com/v8/v8/+/a690aa299414bb3ca2f76e97b02a875e0f721892

[Clemens Hammacher, 2017-02-16 10:47:59]

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

[commit-bot: I haz the power, 2017-02-16 10:48:02]

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-16 10:51:34]

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

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

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/...)

[Clemens Hammacher, 2017-02-16 11:37:23]

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

[commit-bot: I haz the power, 2017-02-16 11:37: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-16 11:39:04]

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

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

Dry run: Try jobs failed on following builders:
  v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  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_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_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_verify_csa_rel_ng/...)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/34911)
  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/22788)

[Clemens Hammacher, 2017-02-16 11:42:05]

Rebase

[Clemens Hammacher, 2017-02-16 11:42:06]

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

[commit-bot: I haz the power, 2017-02-16 11:42: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, 2017-02-16 11:50:44]

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

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

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...)

[Clemens Hammacher, 2017-02-16 20:51:46]

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

[commit-bot: I haz the power, 2017-02-16 20:51:54]

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

[Clemens Hammacher, 2017-02-16 20:55:12]

Rebase

[Clemens Hammacher, 2017-02-16 20:55:13]

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

[commit-bot: I haz the power, 2017-02-16 20:55: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, 2017-02-16 21:06:49]

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

[commit-bot: I haz the power, 2017-02-16 21:06:50]

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...)

[Clemens Hammacher, 2017-02-17 10:19:44]

Remove unused variables and add documentation

[Clemens Hammacher, 2017-02-17 10:19:44]

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

[commit-bot: I haz the power, 2017-02-17 10:19: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-17 10:47:36]

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

[commit-bot: I haz the power, 2017-02-17 10:47:37]

Dry run: This issue passed the CQ dry run.

[Clemens Hammacher, 2017-02-17 10:54:49]

Much green
such refactoring
very performance.
wow!
(PTAL)

[titzer, 2017-02-17 13:38:16]

https://codereview.chromium.org/2696143006/diff/100001/src/wasm/wasm-code-spe...
File src/wasm/wasm-code-specialization.h (right):

https://codereview.chromium.org/2696143006/diff/100001/src/wasm/wasm-code-spe...
src/wasm/wasm-code-specialization.h:62: std::unordered_map<Object*, Object*>
objects_to_relocate;
I think we can replace this with an IdentityMap, which is stable over GC (and
potentially more efficient because it is a hashmap underneath).

[Clemens Hammacher, 2017-02-20 11:05:57]

https://codereview.chromium.org/2696143006/diff/100001/src/wasm/wasm-code-spe...
File src/wasm/wasm-code-specialization.h (right):

https://codereview.chromium.org/2696143006/diff/100001/src/wasm/wasm-code-spe...
src/wasm/wasm-code-specialization.h:62: std::unordered_map<Object*, Object*>
objects_to_relocate;
On 2017/02/17 at 13:38:16, titzer wrote:
> I think we can replace this with an IdentityMap, which is stable over GC (and
potentially more efficient because it is a hashmap underneath).

Also note that I am already using a hash map here.
identity-map.h sais: "The value type {V} must not be a heap type."
Hence we would have to map to Handle<Object>, which would work, but introduces
unnecessary overhead. What do you think?

[titzer, 2017-02-20 11:30:47]

On 2017/02/20 11:05:57, Clemens Hammacher wrote:
>
https://codereview.chromium.org/2696143006/diff/100001/src/wasm/wasm-code-spe...
> File src/wasm/wasm-code-specialization.h (right):
> 
>
https://codereview.chromium.org/2696143006/diff/100001/src/wasm/wasm-code-spe...
> src/wasm/wasm-code-specialization.h:62: std::unordered_map<Object*, Object*>
> objects_to_relocate;
> On 2017/02/17 at 13:38:16, titzer wrote:
> > I think we can replace this with an IdentityMap, which is stable over GC
(and
> potentially more efficient because it is a hashmap underneath).
> 
> Also note that I am already using a hash map here.
> identity-map.h sais: "The value type {V} must not be a heap type."
> Hence we would have to map to Handle<Object>, which would work, but introduces
> unnecessary overhead. What do you think?

Using Handle<Object> sounds fine, as that is more future-proof anyway.

[Clemens Hammacher, 2017-02-20 13:15:10]

Use IdentityMap

[Clemens Hammacher, 2017-02-20 13:15:11]

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

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

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

[Clemens Hammacher, 2017-02-20 13:17:59]

On 2017/02/20 at 11:30:47, titzer wrote:
> On 2017/02/20 11:05:57, Clemens Hammacher wrote:
> >
https://codereview.chromium.org/2696143006/diff/100001/src/wasm/wasm-code-spe...
> > File src/wasm/wasm-code-specialization.h (right):
> > 
> >
https://codereview.chromium.org/2696143006/diff/100001/src/wasm/wasm-code-spe...
> > src/wasm/wasm-code-specialization.h:62: std::unordered_map<Object*, Object*>
> > objects_to_relocate;
> > On 2017/02/17 at 13:38:16, titzer wrote:
> > > I think we can replace this with an IdentityMap, which is stable over GC
(and
> > potentially more efficient because it is a hashmap underneath).
> > 
> > Also note that I am already using a hash map here.
> > identity-map.h sais: "The value type {V} must not be a heap type."
> > Hence we would have to map to Handle<Object>, which would work, but
introduces
> > unnecessary overhead. What do you think?
> 
> Using Handle<Object> sounds fine, as that is more future-proof anyway.

Done. The CodeSpecialization object now requires a Zone, which I decided to not
put inside the CodeSpecialization, in order to be able to reuse it for other
stuff during instantiation.

PTAL.

[commit-bot: I haz the power, 2017-02-20 14:05:06]

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

[commit-bot: I haz the power, 2017-02-20 14:05:07]

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

[titzer, 2017-02-20 15:12:02]

Almost there. Can't in good conscience sign off on crazy union tricks :-)

https://codereview.chromium.org/2696143006/diff/120001/src/wasm/wasm-code-spe...
File src/wasm/wasm-code-specialization.cc (right):

https://codereview.chromium.org/2696143006/diff/120001/src/wasm/wasm-code-spe...
src/wasm/wasm-code-specialization.cc:170: SourcePositionTableIterator
source_pos_it;
Can we heap allocate these instead of using this union pattern?

[Clemens Hammacher, 2017-02-20 15:15:36]

Description was changed from

==========
[wasm] Refactor code specialization / patching

All patching logic is now bundled in one compilation unit.
The CodeSpecialization object is set up by all relocation and patching
that should be applied, and then be run on individual code objects or
the whole instance in one go. We hence only need to iterate all
relocation tables exactly once at instantiation.
Also, we do not patch contexts any more since we do not embed them in
generated code any more.

R=titzer@chromium.org
==========

to

==========
[wasm] Refactor code specialization / patching

All patching logic is now bundled in one compilation unit.
The CodeSpecialization object is set up by all relocation and patching
that should be applied, and then be run on individual code objects or
the whole instance in one go. We hence only need to iterate all
relocation tables exactly once at instantiation.
Also, we do not patch contexts any more since we do not embed them in
generated code any more.

R=titzer@chromium.org
BUG=v8:5991
==========

[Clemens Hammacher, 2017-02-20 15:20:55]

Use heap allocation instead of std::optional replacement

[Clemens Hammacher, 2017-02-20 15:20:56]

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

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

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

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

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_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_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/17776)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/35157)

[Clemens Hammacher, 2017-02-20 15:26:39]

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

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

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

[Clemens Hammacher, 2017-02-20 15:32:57]

https://codereview.chromium.org/2696143006/diff/120001/src/wasm/wasm-code-spe...
File src/wasm/wasm-code-specialization.cc (right):

https://codereview.chromium.org/2696143006/diff/120001/src/wasm/wasm-code-spe...
src/wasm/wasm-code-specialization.cc:170: SourcePositionTableIterator
source_pos_it;
On 2017/02/20 at 15:12:01, titzer wrote:
> Can we heap allocate these instead of using this union pattern?

Sure :)
Done.

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

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

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

Dry run: This issue passed the CQ dry run.

[titzer, 2017-02-20 15:49:39]

lgtm

[Clemens Hammacher, 2017-02-20 15:50:33]

The CQ bit was checked by clemensh@chromium.org

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

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-20 15:52:16]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1487605833205310,
"parent_rev": "67462272919c2ffdba7e0598ce00349a2175ca45", "commit_rev":
"a690aa299414bb3ca2f76e97b02a875e0f721892"}

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

Description was changed from

==========
[wasm] Refactor code specialization / patching

All patching logic is now bundled in one compilation unit.
The CodeSpecialization object is set up by all relocation and patching
that should be applied, and then be run on individual code objects or
the whole instance in one go. We hence only need to iterate all
relocation tables exactly once at instantiation.
Also, we do not patch contexts any more since we do not embed them in
generated code any more.

R=titzer@chromium.org
BUG=v8:5991
==========

to

==========
[wasm] Refactor code specialization / patching

All patching logic is now bundled in one compilation unit.
The CodeSpecialization object is set up by all relocation and patching
that should be applied, and then be run on individual code objects or
the whole instance in one go. We hence only need to iterate all
relocation tables exactly once at instantiation.
Also, we do not patch contexts any more since we do not embed them in
generated code any more.

R=titzer@chromium.org
BUG=v8:5991

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

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

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/v8/v8/+/a690aa299414bb3ca2f76e97b02a875e0f7...