Closed Bug 1355263 Opened 8 years ago Closed 8 years ago

[wasm] different wasm scripts might have non-unique source urls

Categories

(Core :: JavaScript Engine, enhancement)

Product:
Core
Component:
JavaScript Engine
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: yury, Assigned: yury)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Bug 1355263 - Generate better source URL for the wasm module.
59 bytes, text/x-review-board-request
luke
: review+
Details
In wasm module metadata we save filename, which is a filename of the script that instantiated the module. The generate source URL/filename we just append " > wasm" suffix to the filename for the debugger. It will be nice to generate somewhat unique id/filename for the modules. For example: function initWasm(s) { return new WebAssembly.Instance(new WebAssembly.Module(wasmTextToBinary(s))); } o1 = initWasm('(module (func) (export "" 0))'); o2 = initWasm('(module (func) (func) (export "" 1))'); will report to the debugger that all sources have url: "test.js > wasm". And the next example: var g = newGlobal(); g.eval(` function initWasm(s) { return new WebAssembly.Instance(new WebAssembly.Module(wasmTextToBinary(s))); } o1 = initWasm('(module (func) (export "" 0))'); o2 = initWasm('(module (func) (func) (export "" 1))'); `); function isWasm(script) { return script.format === "wasm"; } var dbg = new Debugger(g); var foundScripts = dbg.findScripts().filter(isWasm); print(foundScripts[0].source.url); print(foundScripts[1].source.url); will print "test.js line 2 > eval > wasm" twice. The debugger ignores duplicate script with the same URL and displays only one of them.
Assignee: nobody → ydelendik
Blocks: wasm-tools
Attachment #8856759 - Flags: review?(luke)
Comment on attachment 8856759 [details] Bug 1355263 - Generate better source URL for the wasm module. https://reviewboard.mozilla.org/r/128686/#review131394 ::: js/src/wasm/WasmCode.cpp:400 (Diff revision 1) > CacheableChars::sizeOfExcludingThis(MallocSizeOf mallocSizeOf) const > { > return mallocSizeOf(get()); > } > > +ModuleUUID::ModuleUUID() : data() { nit: { on new line ::: js/src/wasm/WasmCode.cpp:416 (Diff revision 1) > + > + /* put in the variant and version bits */ > + time_hi_and_version &= 0x0FFF; > + time_hi_and_version |= (ModuleUUID::UUIDVersion << 12); > + clock_seq_hi_and_reserved &= 0x3F; > + clock_seq_hi_and_reserved |= 0x80; What's.... all this stuff? Is the SHA1 not sufficient?
Comment on attachment 8856759 [details] Bug 1355263 - Generate better source URL for the wasm module. https://reviewboard.mozilla.org/r/128686/#review131394 > What's.... all this stuff? Is the SHA1 not sufficient? Valid UUID must have version and variant bits set (per RFC 4122). I added a comment about that.
But it seems like we don't have a proper UUID since the bits of the UUID are uniquely determined by the bytecode contents. Looking at Chrome, it seem they just have an 8-character hash; that would perhaps make this look less bulky in the UI, perhaps we could just XOR-fold the SHA1 down to something shorter? Also, Chrome seems to use "wasm://" not "wasm:" as the prefix.
Comment on attachment 8856759 [details] Bug 1355263 - Generate better source URL for the wasm module. https://reviewboard.mozilla.org/r/128686/#review131462 Sorry for the delay! After our last chat this dropped off my radar. One request significant change requested and then I promise I'll re-review with haste. ::: js/src/wasm/WasmCode.h:425 (Diff revision 2) > + uint32_t time_low; > + uint16_t time_mid; > + uint16_t time_hi_and_version; > + uint8_t clock_seq_hi_and_reserved; > + uint8_t clock_seq_low; > + uint8_t node[6]; Since there's no requirement for having an RFC 4122-compliant UUID, I think I'd rather just use a plain char[8] array. This will feel less magical to people modifying the code in the future making it easier to refactor etc. ::: js/src/wasm/WasmCode.cpp:1155 (Diff revision 2) > + // - UUID based on SHA1 hash of the module bytes. > + js::StringBuffer result(cx); > + if (!result.append("wasm:")) > + return nullptr; > + js::StringBuffer filenamePrefix(cx); > + if (EncodeURI(filenamePrefix, metadata().filename.get(), strlen(metadata().filename.get())) && filename can be null ::: js/src/wasm/WasmCode.cpp:1156 (Diff revision 2) > + js::StringBuffer result(cx); > + if (!result.append("wasm:")) > + return nullptr; > + js::StringBuffer filenamePrefix(cx); > + if (EncodeURI(filenamePrefix, metadata().filename.get(), strlen(metadata().filename.get())) && > + !(result.append(filenamePrefix.finishString()) && result.append(":"))) Also, could you break this fat conditional into two: if (!EncodeURI(...)) return null if (!result.append(...)) return null
Attachment #8856759 - Flags: review?(luke)
Comment on attachment 8856759 [details] Bug 1355263 - Generate better source URL for the wasm module. https://reviewboard.mozilla.org/r/128686/#review131462 > Since there's no requirement for having an RFC 4122-compliant UUID, I think I'd rather just use a plain char[8] array. This will feel less magical to people modifying the code in the future making it easier to refactor etc. Replaced by 64-bit hash (first 8 bytes of the SHA1) > Also, could you break this fat conditional into two: > if (!EncodeURI(...)) > return null > if (!result.append(...)) > return null Also improved handling of the bad URI.
Comment on attachment 8856759 [details] Bug 1355263 - Generate better source URL for the wasm module. https://reviewboard.mozilla.org/r/128686/#review135062 Thanks! ::: js/src/wasm/WasmCode.cpp:1089 (Diff revision 3) > + // EncodeURI returns false due to invalid chars or OOM -- fail only > + // during OOM. > + if (!uriEncoded && !cx->isExceptionPending()) > + return nullptr; > + if (!uriEncoded) > + cx->clearPendingException(); // ignore invalid URI Could the logic be: if (!EncodeURI(...)) { if (!cx->isExceptionPending()) return nullptr; cx->clearPendingException(); } if (!results.append ...) return nullptr ::: js/src/wasm/WasmCode.cpp:1096 (Diff revision 3) > + return nullptr; > + } > + > + const ModuleHash& hash = metadata().hash; > + char* buf = JS_smprintf("%02x%02x%02x%02x%02x%02x%02x%02x", > + hash[0], hash[1], hash[2], hash[3], hash[4], hash[5], hash[6], hash[7]); Can you static_assert(sizeof(hash) == 8) and use UniqueChars to own 'buf'? OR, better yet: you could write a for loop over the bytes (ranged for works on arrays, it seems) and use StringBuffer.append(char) to write your own hex, nibble at a time... that's pretty easy and saves a malloc ;) ::: js/src/wasm/WasmGenerator.cpp:1102 (Diff revision 3) > env_->elemSegments.back().elemCodeRangeIndices = Move(codeRangeIndices); > return true; > } > > +void > +ModuleGenerator::generateUuid(const ShareableBytes& bytecode) Can you rename to generateBytecodeHash? ::: js/src/wasm/WasmGenerator.cpp:1108 (Diff revision 3) > +{ > + mozilla::SHA1Sum::Hash hash; > + mozilla::SHA1Sum sha1Sum; > + sha1Sum.update(bytecode.begin(), bytecode.length()); > + sha1Sum.finish(hash); > + MOZ_ASSERT(sizeof(ModuleHash) <= sizeof(mozilla::SHA1Sum::Hash)); can haz static_assert?
Attachment #8856759 - Flags: review?(luke) → review+
Pushed by ydelendik@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/4ed3b236c64c Generate better source URL for the wasm module. r=luke
https://hg.mozilla.org/mozilla-central/rev/4ed3b236c64c
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1362807
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: