[wasm] Canonicalize function signature indices for matching in indirect calls.

[wasm] Canonicalize function signature indices for matching in indirect calls.

R=bradnelson@chromium.org, ahaas@chromium.org, clemensh@chromium.org
BUG=chromium:575167
Committed: https://crrev.com/ff6941966e1eb9258453766da2f436373774cbef
Cr-Commit-Position: refs/heads/master@{#40169}

[titzer, 2016-10-10 16:49:56]

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

[commit-bot: I haz the power, 2016-10-10 16:50:05]

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

[titzer, 2016-10-10 16:51:12]

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

[commit-bot: I haz the power, 2016-10-10 16:51:21]

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

[titzer, 2016-10-10 16:55:02]

Description was changed from

==========
[wasm] Canonicalize function signature indices for matching in indirect calls.

R=bradnelson@chromium.org, ahaas@chromium.org
BUG=chromium:575167
==========

to

==========
[wasm] Canonicalize function signature indices for matching in indirect calls.

R=bradnelson@chromium.org, ahaas@chromium.org, clemensh@chromium.org
BUG=chromium:575167
==========

[titzer, 2016-10-10 16:55:52]

titzer@chromium.org changed reviewers:
+ clemensh@chromium.org

[bradnelson, 2016-10-10 17:32:17]

lgtm

https://codereview.chromium.org/2403093002/diff/20001/test/mjsunit/wasm/indir...
File test/mjsunit/wasm/indirect-calls.js (right):

https://codereview.chromium.org/2403093002/diff/20001/test/mjsunit/wasm/indir...
test/mjsunit/wasm/indirect-calls.js:128: assertEquals(31, module.exports.main(2,
2));
Worth having a test where signatures don't match instead of out of range?

[Clemens Hammacher, 2016-10-10 17:35:03]

https://codereview.chromium.org/2403093002/diff/20001/src/wasm/signature-map.h
File src/wasm/signature-map.h (right):

https://codereview.chromium.org/2403093002/diff/20001/src/wasm/signature-map....
src/wasm/signature-map.h:33: std::map<FunctionSig*, uint32_t,
CompareFunctionSigs> map_;
Depending on the size of the map, an std::unordered_map can be much faster.
Defining Hash and Equals should not be much harder than defining Compare.

https://codereview.chromium.org/2403093002/diff/20001/src/wasm/wasm-interpret...
File src/wasm/wasm-interpreter.cc (right):

https://codereview.chromium.org/2403093002/diff/20001/src/wasm/wasm-interpret...
src/wasm/wasm-interpreter.cc:1423: const WasmIndirectFunctionTable* table =
Maybe place a TODO to think about caching this instead of doing two map lookups
on every call? We could for example patch the code with the canonical index,
which should always be smaller than the present index.

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

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

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

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

[titzer, 2016-10-10 17:45:21]

https://codereview.chromium.org/2403093002/diff/20001/src/wasm/signature-map.h
File src/wasm/signature-map.h (right):

https://codereview.chromium.org/2403093002/diff/20001/src/wasm/signature-map....
src/wasm/signature-map.h:33: std::map<FunctionSig*, uint32_t,
CompareFunctionSigs> map_;
On 2016/10/10 17:35:02, Clemens Hammacher wrote:
> Depending on the size of the map, an std::unordered_map can be much faster.
> Defining Hash and Equals should not be much harder than defining Compare.

I've added a TODO here (since it was much easier to copy the previous
well-tested implementation from wasm-module-builder.cc).

https://codereview.chromium.org/2403093002/diff/20001/src/wasm/wasm-interpret...
File src/wasm/wasm-interpreter.cc (right):

https://codereview.chromium.org/2403093002/diff/20001/src/wasm/wasm-interpret...
src/wasm/wasm-interpreter.cc:1423: const WasmIndirectFunctionTable* table =
On 2016/10/10 17:35:03, Clemens Hammacher wrote:
> Maybe place a TODO to think about caching this instead of doing two map
lookups
> on every call? We could for example patch the code with the canonical index,
> which should always be smaller than the present index.

Added a TODO here.

https://codereview.chromium.org/2403093002/diff/20001/test/mjsunit/wasm/indir...
File test/mjsunit/wasm/indirect-calls.js (right):

https://codereview.chromium.org/2403093002/diff/20001/test/mjsunit/wasm/indir...
test/mjsunit/wasm/indirect-calls.js:128: assertEquals(31, module.exports.main(2,
2));
On 2016/10/10 17:32:17, bradnelson wrote:
> Worth having a test where signatures don't match instead of out of range?

Done.

[titzer, 2016-10-10 17:48:50]

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

[commit-bot: I haz the power, 2016-10-10 17:48: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-10-10 18:14:04]

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

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

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

[ahaas, 2016-10-11 09:20:09]

lgtm with nits

https://codereview.chromium.org/2403093002/diff/40001/src/wasm/signature-map.cc
File src/wasm/signature-map.cc (right):

https://codereview.chromium.org/2403093002/diff/40001/src/wasm/signature-map....
src/wasm/signature-map.cc:11: uint32_t SignatureMap::Get(FunctionSig* sig) {
I think the name {Get} is misleading because typically get-function do not
change the state. What about {FindOrInsert}?

https://codereview.chromium.org/2403093002/diff/40001/src/wasm/wasm-module.h
File src/wasm/wasm-module.h (right):

https://codereview.chromium.org/2403093002/diff/40001/src/wasm/wasm-module.h#...
src/wasm/wasm-module.h:138: bool exported;                // true if exported.
The other fields do not end with "_".

https://codereview.chromium.org/2403093002/diff/40001/src/wasm/wasm-module.h#...
src/wasm/wasm-module.h:139: SignatureMap map_;            // canonicalizing map
for sig indexes.
Is it really clear that {map} is a SignatureMap? If not, could you call the
field signature_map?

https://codereview.chromium.org/2403093002/diff/40001/test/cctest/wasm/test-r...
File test/cctest/wasm/test-run-wasm.cc (right):

https://codereview.chromium.org/2403093002/diff/40001/test/cctest/wasm/test-r...
test/cctest/wasm/test-run-wasm.cc:2666: CHECK_EQ(88, r.Call(0));
Also here, could you use t*.function_index()?

https://codereview.chromium.org/2403093002/diff/40001/test/mjsunit/wasm/indir...
File test/mjsunit/wasm/indirect-calls.js (right):

https://codereview.chromium.org/2403093002/diff/40001/test/mjsunit/wasm/indir...
test/mjsunit/wasm/indirect-calls.js:123: assertEquals(33, module.exports.main(1,
0));
Could you use function index created by builder.addFunction here instead of just
0, 1, 2?

[titzer, 2016-10-11 11:58:31]

https://codereview.chromium.org/2403093002/diff/40001/src/wasm/signature-map.cc
File src/wasm/signature-map.cc (right):

https://codereview.chromium.org/2403093002/diff/40001/src/wasm/signature-map....
src/wasm/signature-map.cc:11: uint32_t SignatureMap::Get(FunctionSig* sig) {
On 2016/10/11 09:20:09, ahaas wrote:
> I think the name {Get} is misleading because typically get-function do not
> change the state. What about {FindOrInsert}?

Done.

https://codereview.chromium.org/2403093002/diff/40001/src/wasm/wasm-module.h
File src/wasm/wasm-module.h (right):

https://codereview.chromium.org/2403093002/diff/40001/src/wasm/wasm-module.h#...
src/wasm/wasm-module.h:138: bool exported;                // true if exported.
On 2016/10/11 09:20:09, ahaas wrote:
> The other fields do not end with "_".

Done.

https://codereview.chromium.org/2403093002/diff/40001/src/wasm/wasm-module.h#...
src/wasm/wasm-module.h:139: SignatureMap map_;            // canonicalizing map
for sig indexes.
On 2016/10/11 09:20:09, ahaas wrote:
> Is it really clear that {map} is a SignatureMap? If not, could you call the
> field signature_map?

Leaving for now.

https://codereview.chromium.org/2403093002/diff/40001/test/cctest/wasm/test-r...
File test/cctest/wasm/test-run-wasm.cc (right):

https://codereview.chromium.org/2403093002/diff/40001/test/cctest/wasm/test-r...
test/cctest/wasm/test-run-wasm.cc:2666: CHECK_EQ(88, r.Call(0));
On 2016/10/11 09:20:09, ahaas wrote:
> Also here, could you use t*.function_index()?

Done.

https://codereview.chromium.org/2403093002/diff/40001/test/mjsunit/wasm/indir...
File test/mjsunit/wasm/indirect-calls.js (right):

https://codereview.chromium.org/2403093002/diff/40001/test/mjsunit/wasm/indir...
test/mjsunit/wasm/indirect-calls.js:123: assertEquals(33, module.exports.main(1,
0));
On 2016/10/11 09:20:09, ahaas wrote:
> Could you use function index created by builder.addFunction here instead of
just
> 0, 1, 2?

Done.

[titzer, 2016-10-11 12:02:29]

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

[commit-bot: I haz the power, 2016-10-11 12:02:33]

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-10-11 12:35:05]

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

[commit-bot: I haz the power, 2016-10-11 12:35:06]

Dry run: This issue passed the CQ dry run.

[titzer, 2016-10-11 12:38:01]

The CQ bit was checked by titzer@chromium.org

[titzer, 2016-10-11 12:38:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
bradnelson@chromium.org, ahaas@chromium.org
Link to the patchset: https://codereview.chromium.org/2403093002/#ps80001
(title: "Address review comments")

[commit-bot: I haz the power, 2016-10-11 12:38:07]

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-10-11 12:40:27]

Description was changed from

==========
[wasm] Canonicalize function signature indices for matching in indirect calls.

R=bradnelson@chromium.org, ahaas@chromium.org, clemensh@chromium.org
BUG=chromium:575167
==========

to

==========
[wasm] Canonicalize function signature indices for matching in indirect calls.

R=bradnelson@chromium.org, ahaas@chromium.org, clemensh@chromium.org
BUG=chromium:575167
==========

[commit-bot: I haz the power, 2016-10-11 12:40:28]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-10-11 12:40:36]

Description was changed from

==========
[wasm] Canonicalize function signature indices for matching in indirect calls.

R=bradnelson@chromium.org, ahaas@chromium.org, clemensh@chromium.org
BUG=chromium:575167
==========

to

==========
[wasm] Canonicalize function signature indices for matching in indirect calls.

R=bradnelson@chromium.org, ahaas@chromium.org, clemensh@chromium.org
BUG=chromium:575167

Committed: https://crrev.com/ff6941966e1eb9258453766da2f436373774cbef
Cr-Commit-Position: refs/heads/master@{#40169}
==========

[commit-bot: I haz the power, 2016-10-11 12:40:38]

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