1647791 - experiment to see whether we could retroactively disallow duplicate imports

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, task, P3)

Description

[Luke Wagner [:luke]]

The MVP spec currently disallows duplicate exports, but not duplicate imports. This raises a few tricky design questions in the context of module-linking (where we want module subtyping to allow the reordering of fields and where two-level imports get reinterpreted as single-level imports of instances; see module-linking/#7.) Rather than adding complexity to deal with duplicates, it'd be lovely if we could retroactively disallow duplicate imports, but it's not clear whether this would break sites.

In this bug, it'd be great to:

1. write a patch to do quick-n-dirty testing of all the wasm apps known to us
2. if step 1 is positive, poll the CG and if the CG doesn't shoot it down, try to land the restriction and see if anything breaks

Comment 1

[Lars T Hansen [:lth]]

What happens when A and B both import $f from C as $a and $b respectively, which they then re-export, and then D imports $a and $b from A and B? Reasonable notions of composition will require this to continue to work I think, but it's not clear to me that D should be required to treat $a and $b as any different from $f, other than in the sense of having different names. So what's the precise meaning of "duplicate"?

Comment 2

[Alex Crichton [:acrichto]]

Oh by "duplicate" I believe Luke means the literal strings in the wasm module, for example this is allowed today:

(module
  (import "" "" (func))
  (import "" "" (global i32)) ;; same import names as the `func`
)

In practice, though, it's unlikely anyone really uses this, so I think the idea is seeing what breaks if an import module/name pair can only show up once-per-module

Comment 3

[rossberg]

FWIW, the main use case for duplicate imports that I'm aware of is emulating a form of polymorphic import, like this:

(module
  (func $print_i32 (import "sys" "print") (param i32))
  (func $print_i64 (import "sys" "print") (param i64))
)

We actually had an API like that for a while, but moved away from it. But I wouldn't be surprised if the same idea has been used elsewhere.

Comment 4

[Luke Wagner [:luke]]

Yeah, I know I've done that (comment 3) before when, e.g., importing console.log in ad hoc tests, so I'm not holding my breath. But if we could get away with this, it certainly would make things simpler.

Comment 5

[Mike Conca [:mconca]]

(In reply to Luke Wagner [:luke] from comment #4)

Yeah, I know I've done that (comment 3) before when, e.g., importing console.log in ad hoc tests, so I'm not holding my breath. But if we could get away with this, it certainly would make things simpler.

Can we land a probe to detect this situation (duplicate imports) and look at the frequency of occurrence in the wild?

Comment 6

[Luke Wagner [:luke]]

Yep, that's what I propose as "step 2" in comment 0 (step 1 is to avoid us wasting time if we can trivially find examples).

Comment 7

[Luke Wagner [:luke]]

Attachment: Bug 1647791 - WebAssembly: add telemetry for duplicate imports r=lth!

Comment 8

[Luke Wagner [:luke]]

Oops, that took me a while to get to. I tested the patch on a bunch of different uses of wasm from different toolchains and nothing seems to use duplicate imports, which is encouraging. Most wasm apps use Emscripten/LLVM, which Alon said doesn't intentionally generate duplicates. I also used about:telemetry and an artificial wasm module containing duplicates to confirm that USE_COUNTER2_JS_WASM_DUPLICATE_IMPORTS (under Histograms > Content) does indeed get bumped on duplicates.

Comment 9

[Lars T Hansen [:lth]]

Comment on attachment 9194954 [details]
Bug 1647791 - WebAssembly: add telemetry for duplicate imports r=lth!

I guess we need data collection approval to land this?

Comment 10

[Chris H-C :chutten]

Comment on attachment 9194954 [details]
Bug 1647791 - WebAssembly: add telemetry for duplicate imports r=lth!

Sure do! You can find the process over here: https://wiki.mozilla.org/Firefox/Data_Collection

(Fill out the form, attach it to the bug, mark data-review?, etc.)

Comment 11

[Luke Wagner [:luke]]

Attachment: request.txt

Comment 12

[Chris H-C :chutten]

Comment on attachment 9195982 [details]
request.txt

DATA COLLECTION REVIEW RESPONSE:

Is there or will there be documentation that describes the schema for the ultimate data set available publicly, complete and accurate?

Yes.

Is there a control mechanism that allows the user to turn the data collection on and off?

Yes. This collection is Telemetry so can be controlled through Firefox's Preferences.

If the request is for permanent data collection, is there someone who will monitor the data over time?

No. This collection will expire on May 1, 2021.

Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under?

Category 1, Technical.

Is the data collection request for default-on or default-off?

Default on for all channels.

Does the instrumentation include the addition of any new identifiers?

No.

Is the data collection covered by the existing Firefox privacy notice?

Yes.

Does there need to be a check-in in the future to determine whether to renew the data?

Yes. :luke is responsible for renewing or removing the collection before the end of April.

---

Result: datareview+

Comment 13

[Luke Wagner [:luke]]

Thanks! Lars: do you think this is ready to land? If so, could you Lando it? I don't seem to have permission anymore :P

Comment 14

[Lars T Hansen [:lth]]

I think we can land. I'll try to do it today, monday at the latest.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/17d5cf143c0c
WebAssembly: add telemetry for duplicate imports r=lth

Comment 16

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/17d5cf143c0c