Embedding limits #873

titzer · 2018-09-13T12:53:24Z

This PR contains additional spec text and tests for embedding limits as discussed in #607.

binji · 2018-09-13T20:03:29Z

test/js-api/limits.js

+        builder.setFunctionTableBounds(1, 1);
+        let array = [];
+        for (let i = 0; i < count; i++) {
+            builder.addFunctionTableInit(0, false, array, false);


This terminology confused me at first; "FunctionTableInit" here is what the spec calls an "Element Segment". I've opened a PR to rename these: #874

binji · 2018-09-13T20:06:09Z

test/js-api/limits.js

+let kJSEmbeddingMaxFunctionParams = 1000;
+let kJSEmbeddingMaxFunctionReturns = 1;
+let kJSEmbeddingMaxTableSize = 10000000;
+let kJSEmbeddingMaxTableEntries = 10000000;


This should probably be called kJSEmbeddingMaxElementSegments based on the test below.

dead-claudia · 2018-09-14T02:24:15Z

document/js-api/index.bs

+
+The WebAssembly core specification allows an implementation to define limits on the syntactic structure of the module.
+The standard WebAssembly JavaScript Interface described in this document hereby defines a set of standard limits on the syntactic module structure as below.
+Modules that exceed these limits are invalid and are rejected with a {{CompileError}}.


I presume it's implied that lower implementation limits may exist?

I think the idea here is that there actually shouldn't be lower implementation limits, that these are exact. Should the text be clarified?

Probably, since low-resource devices (like embedded) and computationally-sensitive uses (think: Etherum's eWASM) won't be able to cope with these limits particularly well.

These limits only apply to JavaScript embeddings and are not relevant to others, including blockchain platforms.

@rossberg What about JS embeddings without sufficient RAM? For one concrete example, mobile devices might have performance problems processing large WASM bundles due to a slower clock speed + less RAM available. (The max amount of allocatable memory per this, for example, is 2GB. Low-end smartphones don't usually have this much available - some only have 2GB of RAM.)

PTAL at new text

littledan · 2018-09-15T14:36:00Z

Cc @Ms2ger if you could review these tests, it would be great.

littledan · 2018-09-14T14:53:34Z

document/js-api/index.bs

+<h2 id="limits">Implementation-defined Limits</h2>
+
+The WebAssembly core specification allows an implementation to define limits on the syntactic structure of the module.
+The standard WebAssembly JavaScript Interface described in this document hereby defines a set of standard limits on the syntactic module structure as below.


The word "hereby" is not used like this in standards very much; I'd eliminate it (or use formally defined terms like "SHOULD" or "MUST"). But this is editorial and can be done in a follow-on editorial patch.

littledan · 2018-09-14T15:07:04Z

document/js-api/index.bs

+<li>The initial or maximum number of pages for any memory, declared or imported, is at most 32767.</li>
+
+<li>The maximum number of parameters to any function is 1000.</li>
+<li>The maximum number of return types for any function is 1.</li>


s/types/values/? Again, an editorial change that we can consider in a follow-on PR.

littledan · 2018-09-15T14:26:02Z

test/build.py

@@ -1,4 +1,4 @@
-#!/usr/bin/env python3
+#!/usr/bin/env python


Not sure if this change was intentional

littledan · 2018-09-15T14:27:19Z

document/js-api/index.bs

+
+The WebAssembly core specification allows an implementation to define limits on the syntactic structure of the module.
+The standard WebAssembly JavaScript Interface described in this document hereby defines a set of standard limits on the syntactic module structure as below.
+Modules that exceed these limits are invalid and are rejected with a {{CompileError}}.


I think the idea here is that there actually shouldn't be lower implementation limits, that these are exact. Should the text be clarified?

littledan · 2018-09-15T14:32:28Z

test/js-api/limits.js

+if (failures.length > 0) {
+  throw failures[0];
+}
+//=======================================================================


Does this part need to be removed? (And the similar segment above)

Ms2ger · 2018-09-17T08:23:33Z

document/js-api/index.bs

+
+The WebAssembly core specification allows an implementation to define limits on the syntactic structure of the module.
+The standard WebAssembly JavaScript Interface described in this document hereby defines a set of standard limits on the syntactic module structure as below.
+Modules that exceed these limits are invalid and are rejected with a {{CompileError}}.


This is a "Statement of fact" with no normative prose to back it up.

It's also a "COMEFROM" statement; it's not in the place where you look when for a definition of compiling or validating modules. Perhaps a better place would be in #compile-a-webassembly-module. We'd need to make sure that HTML calls through that, then.

It's perhaps worth elaborating on why this is in js-api, though, since that seems like a weird layering on first sight.

Ms2ger · 2018-09-17T08:24:13Z

test/js-api/limits.js

+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+*/


Please submit the test to wpt directly, under its license(s).

I don't think we agreed that all tests need to be submitted directly to wpt; they can be added through either side.

Ms2ger · 2018-09-17T08:24:26Z

test/js-api/limits.js

+let kTestAsyncCompile = true;
+
+//=======================================================================
+// HARNESS SNIPPET, DO NOT COMMIT


Do not commit.

Ms2ger · 2018-09-17T08:24:37Z

test/js-api/limits.js

+let kJSEmbeddingMaxMemories = 1;
+
+function verbose(...args) {
+  if (false) print(...args);


Don't use print().

Is there a better alternative? I've found it helpful to have console/print output while developing the tests. Any suggestion?

Ms2ger · 2018-09-17T08:25:07Z

test/js-api/limits.js

+
+let kTestValidate = true;
+let kTestSyncCompile = true;
+let kTestAsyncCompile = true;


Remove these; split into three files if needed.

I think I'd prefer not to split this into three files. The flags were useful in developing the test. Is that worth keeping in your opinion?

Ms2ger · 2018-09-17T08:37:03Z

test/js-api/limits.js

+}
+//=======================================================================
+
+function testLimit(name, min, limit, gen) {


Some documentation, please.

Ms2ger · 2018-09-17T09:37:00Z

document/js-api/index.bs

+
+<li>The maximum number of parameters to any function is 1000.</li>
+<li>The maximum number of return types for any function is 1.</li>
+<li>The maximum size of a function body, including locals declarations, is 7654321 bytes.</li>


Is that really the number you want to go with?

Ms2ger · 2018-09-17T09:42:50Z

test/js-api/limits.js

+      })
+      .catch(err => {
+        if (expected) {
+          let msg = `UNEXPECTED FAIL: ${name} == ${count} (${e})`;


Same question about putting this in the same function.

Ms2ger · 2018-09-17T09:53:22Z

test/js-api/limits.js

+          });
+
+// TODO(titzer): ugh, that's hard to test.
+DISABLED.testLimit("module size", 1, kJSEmbeddingMaxModuleSize,


File an issue and remove this, please.

Ms2ger · 2018-09-17T09:54:17Z

test/js-api/limits.js

+        let type = builder.addType(kSig_v_v);
+        let nops = count-2;
+        let array = new Array(nops);
+        for (let i = 0; i < nops; i++) array[i] = kExprNop;


I think array.fill() is probably well-supported enough, if you want.

littledan

Spec text LGTM. I'll leave it to @titzer to decide whether he wants to fix the test issues that @Ms2ger identified in this PR or a follow-on one.

littledan · 2018-09-24T20:14:59Z

test/js-api/limits.js

+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+*/


I don't think we agreed that all tests need to be submitted directly to wpt; they can be added through either side.

littledan · 2018-10-09T20:54:37Z

This PR has very important spec text material, so I'm landing it; @Ms2ger , could you clean up the tests in a follow-on patch?

Ben L. Titzer added 6 commits April 27, 2018 16:01

Initial commit of spec text and test for WASM limits.

5808393

Add test for function with params and locals

5f9a33d

Update limits test and harness

977f22c

Add sync compile tests

a97b6cb

Mention that parameters are included in the local computation

b51d028

Merge branch 'master' into embedding_limits

b430993

binji reviewed Sep 13, 2018

View reviewed changes

dead-claudia reviewed Sep 14, 2018

View reviewed changes

littledan reviewed Sep 15, 2018

View reviewed changes

Ms2ger requested changes Sep 17, 2018

View reviewed changes

Ben L. Titzer added 2 commits September 24, 2018 13:53

Merge branch 'master' into embedding_limits

3605c4e

Address first review comments

190b415

littledan approved these changes Sep 24, 2018

View reviewed changes

littledan merged commit ea78863 into master Oct 9, 2018

binji deleted the embedding_limits branch October 9, 2018 22:09

littledan mentioned this pull request Dec 13, 2018

Aligning on implementation-specific limits. #607

Closed

lars-t-hansen mentioned this pull request Apr 23, 2019

Problematic sentinel value with table.grow WebAssembly/reference-types#40

Closed

bvisness mentioned this pull request Jan 13, 2025

[js-api] Ambiguous and inconsistent handling of implementation limits #1863

Open

Embedding limits #873

Embedding limits #873

Conversation

titzer commented Sep 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

littledan commented Sep 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

littledan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

littledan commented Oct 9, 2018