Skip to content

Pass the blob URL for preloads in WasmEMCCBenchmark. #74

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion 8bitbench/benchmark.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,14 +34,24 @@ function dumpFrame(vec) {

class Benchmark {
isInstantiated = false;
romBinary;

async init() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC, this is essentially shared code with the getBinary function in WasmEMCCBenchmark below, just that there it's only used for initializing the Module["wasmBinary"] = getBinary(...) and here it's for an arbitrary (non-Wasm) file, that happens to be the emulator ROM, right?

Can we unify this, ideally also with the mechansim for preloading blogs of JavaScript line items (ARES-6/Babylon below)?

if (isInBrowser) {
let response = await fetch(romBinary);
this.romBinary = new Int8Array(await response.arrayBuffer());
} else {
this.romBinary = new Int8Array(read(romBinary, "binary"));
}
}

async runIteration() {
if (!this.isInstantiated) {
await wasm_bindgen(Module.wasmBinary);
this.isInstantiated = true;
}

wasm_bindgen.loadRom(Module.romBinary);
wasm_bindgen.loadRom(this.romBinary);

const frameCount = 2 * 60;
for (let i = 0; i < frameCount; ++i) {
Expand Down
8 changes: 4 additions & 4 deletions ARES-6/Babylon/benchmark.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,10 @@ class Benchmark {
let sources = [];

const files = [
[isInBrowser ? airBlob : "./ARES-6/Babylon/air-blob.js", {}]
, [isInBrowser ? basicBlob : "./ARES-6/Babylon/basic-blob.js", {}]
, [isInBrowser ? inspectorBlob : "./ARES-6/Babylon/inspector-blob.js", {}]
, [isInBrowser ? babylonBlob : "./ARES-6/Babylon/babylon-blob.js", {sourceType: "module"}]
[airBlob, {}]
, [basicBlob, {}]
, [inspectorBlob, {}]
, [babylonBlob, {sourceType: "module"}]
];

for (let [file, options] of files) {
Expand Down
17 changes: 6 additions & 11 deletions Dart/benchmark.js
Original file line number Diff line number Diff line change
Expand Up @@ -266,19 +266,14 @@ class Benchmark {
// is a map from module variable name (which will hold the resulting module
// namespace object) to relative module URL, which is resolved in the
// `preRunnerCode`, similar to this code here.
if (isInBrowser) {
// In browsers, relative imports don't work since we are not in a module.
// (`import.meta.url` is not defined.)
const pathname = location.pathname.match(/^(.*\/)(?:[^.]+(?:\.(?:[^\/]+))+)?$/)[1];
this.dart2wasmJsModule = await import(location.origin + pathname + "./Dart/build/flute.dart2wasm.mjs");
} else {

try {
this.dart2wasmJsModule = await import(jsModule);
} catch {
// In shells, relative imports require different paths, so try with and
// without the "./" prefix (e.g., JSC requires it).
try {
this.dart2wasmJsModule = await import("Dart/build/flute.dart2wasm.mjs");
} catch {
this.dart2wasmJsModule = await import("./Dart/build/flute.dart2wasm.mjs");
}
if (!isInBrowser)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So what happens in !isInBrowser environments, where the previous dynamic import threw an exception? In that case, wouldn't we simply not set this.dart2wasmJsModule and fail later? Or do we assume that we only reach here if isInBrowser == false? In that case, can we instead change this to an explicit assertion, e.g.

console.assert(!isInBrowser, "relative imports should always succeed in browsers, this code is only for shells");

or something.

this.dart2wasmJsModule = await import(jsModule.slice("./".length))
}
}

Expand Down
86 changes: 39 additions & 47 deletions JetStreamDriver.js
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,6 @@ class Driver {
}

benchmark.updateUIAfterRun();
console.log(benchmark.name)

if (isInBrowser) {
const cache = JetStream.blobDataCache;
Expand Down Expand Up @@ -776,8 +775,8 @@ class Benchmark {

if (this.plan.preload) {
let str = "";
for (let [variableName, blobUrl] of this.preloads)
str += `const ${variableName} = "${blobUrl}";\n`;
for (let [ variableName, blobURLOrPath ] of this.preloads)
str += `const ${variableName} = "${blobURLOrPath}";\n`;
addScript(str);
}

Expand Down Expand Up @@ -994,9 +993,16 @@ class Benchmark {
if (this._resourcesPromise)
return this._resourcesPromise;

const filePromises = !isInBrowser ? this.plan.files.map((file) => fileLoader.load(file)) : [];
this.preloads = [];
this.blobs = [];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAICT, this.blobs is never read, can't this be removed?


const promise = Promise.all(filePromises).then((texts) => {
if (isInBrowser) {
this._resourcesPromise = Promise.resolve();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't fully understood the (somewhat intertwined, complex) preload, resource loading code. At a high-level, why do we just early return here having added anything to this.scripts?

return this._resourcesPromise;
}

const filePromises = this.plan.files.map((file) => fileLoader.load(file));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get this in conjunction with fileLoader._loadInternal: Here, we never reach if isInBrowser, because of the early return above. But in fileLoader._loadInternal (from line 199,

async _loadInternal(url) {
if (!isInBrowser)
return Promise.resolve(readFile(url));
let response;
const tries = 3;
while (tries--) {
let hasError = false;
try {
response = await fetch(url);
} catch (e) {
hasError = true;
}
if (!hasError && response.ok)
break;
if (tries)
continue;
globalThis.allIsGood = false;
throw new Error("Fetch failed");
}
if (url.indexOf(".js") !== -1)
return response.text();
else if (url.indexOf(".wasm") !== -1)
return response.arrayBuffer();
throw new Error("should not be reached!");
}
) we early return just early return if (!isInBrowser) Promise.resolve(readFile(url)). In other words, isn't this whole remaining code of fileLoader superfluous?

this._resourcesPromise = Promise.all(filePromises).then((texts) => {
if (isInBrowser)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The if (isInBrowser) return; in lines 1006/1007 is dead code now, because of line 999, right?

return;
this.scripts = [];
Expand All @@ -1005,10 +1011,11 @@ class Benchmark {
this.scripts.push(text);
});

this.preloads = [];
this.blobs = [];
if (this.plan.preload) {
for (const prop of Object.getOwnPropertyNames(this.plan.preload))
this.preloads.push([ prop, this.plan.preload[prop] ]);
}

this._resourcesPromise = promise;
return this._resourcesPromise;
}

Expand Down Expand Up @@ -1167,7 +1174,7 @@ class AsyncBenchmark extends DefaultBenchmark {
// part of a larger project's build system or a wasm benchmark compiled from a language that doesn't compile with emcc.
class WasmEMCCBenchmark extends AsyncBenchmark {
get prerunCode() {
let str = `
return `
let verbose = false;

let globalObject = this;
Expand Down Expand Up @@ -1198,58 +1205,42 @@ class WasmEMCCBenchmark extends AsyncBenchmark {
},
};
globalObject.Module = Module;
`;
return str;
`;
}

// FIXME: Why is this part of the runnerCode and not prerunCode?
// This is in runnerCode rather than prerunCode because prerunCode isn't currently structured to be async by default.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the difference between prerunCode() and Benchmark.init()? The latter is async / called with await for AsyncBenchmark, see

await __benchmark.init();

Could one unify the two, or move this code into async init()?

get runnerCode() {
let str = `function loadBlob(key, path, andThen) {`;

let str = `(async function doRunWrapper() {`
if (isInBrowser) {
str += `
var xhr = new XMLHttpRequest();
xhr.open('GET', path, true);
xhr.responseType = 'arraybuffer';
xhr.onload = function() {
Module[key] = new Int8Array(xhr.response);
andThen();
};
xhr.send(null);
async function getBinary(key, blobURL) {
const response = await fetch(blobURL);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I said above, this getBinary function is only used for the Wasm binary of Emscripten benchmarks, but could we use some more general mechanism from the JavaScript line items?

Module[key] = new Int8Array(await response.arrayBuffer());
}
`;
} else {
} else
str += `
Module[key] = new Int8Array(read(path, "binary"));

Module.setStatus = null;
Module.monitorRunDependencies = null;

Promise.resolve(42).then(() => {
try {
andThen();
} catch(e) {
console.log("error running wasm:", e);
console.log(e.stack);
throw e;
// Needed because SpiderMonkey shell doesn't have a setTimeout.
Module.setStatus = null;
Module.monitorRunDependencies = null;
function getBinary(key, path) {
Module[key] = new Int8Array(read(path, "binary"));
}
})
`;
}

str += "}";

let keys = Object.keys(this.plan.preload);
for (let i = 0; i < keys.length; ++i) {
str += `loadBlob("${keys[i]}", "${this.plan.preload[keys[i]]}", async () => {\n`;
for (let [ preloadKey, blobURLOrPath ] of this.preloads) {
if (preloadKey == "wasmBinary") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we get rid of this special casing of just the wasmBinary preloadKey? There are not that many Emscripten workloads, so I'd be happy to just add a line Module["wasmBinary"] = genericPreloadFunc(wasmBinary) to every one of those (for a little less "magic").

str += `await getBinary("${preloadKey}", "${blobURLOrPath}");\n`
break;
}
}

str += super.runnerCode;
for (let i = 0; i < keys.length; ++i) {
str += `})`;
}
str += `;`;

str += "\n})().catch((error) => { top.currentReject(error); });"

return str;

}
};

Expand Down Expand Up @@ -2091,7 +2082,8 @@ let BENCHMARKS = [
"./Dart/benchmark.js",
],
preload: {
wasmBinary: "./Dart/build/flute.dart2wasm.wasm"
jsModule: "./Dart/build/flute.dart2wasm.mjs",
wasmBinary: "./Dart/build/flute.dart2wasm.wasm",
},
iterations: 15,
worstCaseCount: 2,
Expand Down
2 changes: 1 addition & 1 deletion code-load/code-first-load.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ class Benchmark {

inspectorText = request.responseText;
} else
inspectorText = readFile("./code-load/inspector-payload-minified.js");
inspectorText = readFile(inspectorPayloadBlob);

this.inspectorText = `let _____top_level_____ = ${Math.random()}; ${inspectorText}`;

Expand Down
2 changes: 1 addition & 1 deletion code-load/code-multi-load.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ class Benchmark {
throw new Error("Expect non-empty sources");
inspectorText = request.responseText;
} else
inspectorText = readFile("./code-load/inspector-payload-minified.js");
inspectorText = readFile(inspectorPayloadBlob);

this.inspectorText = `let _____top_level_____ = ${Math.random()}; ${inspectorText}`;
this.index = 0;
Expand Down
Loading