Our stack traces aren't great because: - They don't include a way to figure ouch which module the function is in - They treat stubs the same as user functions - They don't honor module name from name section, if present (which right now is never the case because toolchains don't generate the name section) This makes it annoying to debug code. Further, the proposed design has slightly changed since we originally implemented this: https://github.com/WebAssembly/design/blob/master/Web.md#developer-facing-display-conventions
Created attachment 326154 [details] patch
Created attachment 326157 [details] patch
Attachment 326157 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/wasm/WasmNameSection.h:29: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 17 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 326157 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=326157&action=review r=me > Source/JavaScriptCore/wasm/WasmModuleInformation.cpp:47 > + , hash(sha1(source)) should we do this lazily? > Source/JavaScriptCore/wasm/WasmNameSection.h:40 > + for (size_t i = 0; i != hash.length(); ++i) nit: we usually use < instead of != here > Source/JavaScriptCore/wasm/WasmNameSection.h:43 > + NameSection() = delete; Isn't this implicit in you defining the above constructor? > Source/JavaScriptCore/wasm/WasmNameSection.h:44 > + NameSection(const NameSection&) = delete; WTF_MAKE_NONCOPYABLE?
Created attachment 328047 [details] patch (In reply to Saam Barati from comment #4) > Comment on attachment 326157 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=326157&action=review > > r=me > > > Source/JavaScriptCore/wasm/WasmModuleInformation.cpp:47 > > + , hash(sha1(source)) > > should we do this lazily? Nah, I don't think it's worth it. I think we'll eventually want to drop sections which aren't referenced, so hashing won't be possible that late and we'll have to un-lazy. > > Source/JavaScriptCore/wasm/WasmNameSection.h:40 > > + for (size_t i = 0; i != hash.length(); ++i) > > nit: we usually use < instead of != here Done. > > Source/JavaScriptCore/wasm/WasmNameSection.h:43 > > + NameSection() = delete; > > Isn't this implicit in you defining the above constructor? Yeah, I was just being explicit, but I can remove. > > Source/JavaScriptCore/wasm/WasmNameSection.h:44 > > + NameSection(const NameSection&) = delete; > > WTF_MAKE_NONCOPYABLE? I'm not a fan of macros that replace a clear one-line thing :-)
Comment on attachment 326157 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=326157&action=review >>> Source/JavaScriptCore/wasm/WasmNameSection.h:44 >>> + NameSection(const NameSection&) = delete; >> >> WTF_MAKE_NONCOPYABLE? > > I'm not a fan of macros that replace a clear one-line thing :-) Except the macro is doing more than what you're doing by hand. You aren't deleting the copy assignment operator. So I think you might not actually getting the guarantee I believe you want. Also, regarding your motivation for skipping the macro: I disagree. The macro gives a name to this action. It's much more understandable because the name is obvious. As a reader of this code, even if you didn't understand C++ that well, the name of the macro describes what's going on. This syntax won't make sense to somebody who doesn't have at least decent understanding of C++.
Comment on attachment 326157 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=326157&action=review >>>> Source/JavaScriptCore/wasm/WasmNameSection.h:44 >>>> + NameSection(const NameSection&) = delete; >>> >>> WTF_MAKE_NONCOPYABLE? >> >> I'm not a fan of macros that replace a clear one-line thing :-) > > Except the macro is doing more than what you're doing by hand. You aren't deleting the copy assignment operator. So I think you might not actually getting the guarantee I believe you want. > > Also, regarding your motivation for skipping the macro: I disagree. The macro gives a name to this action. It's much more understandable because the name is obvious. As a reader of this code, even if you didn't understand C++ that well, the name of the macro describes what's going on. This syntax won't make sense to somebody who doesn't have at least decent understanding of C++. I guess you could just want to prevent copy construction but not copy assignment, but that seems weird.
Comment on attachment 328047 [details] patch Clearing flags on attachment: 328047 Committed r225378: <https://trac.webkit.org/changeset/225378>
All reviewed patches have been landed. Closing bug.
<rdar://problem/35788939>
Comment on attachment 328047 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=328047&action=review > Source/JavaScriptCore/wasm/WasmIndexOrName.cpp:49 > + return String("wasm-stub"); You should use `ASCIILiteral("wasm-stub")` in this case to prevent a copy: https://trac.webkit.org/wiki/EfficientStrings > Source/JavaScriptCore/wasm/WasmIndexOrName.cpp:53 > + return makeString(moduleName, ".wasm-function[", String::number(ion.m_indexName.index & ~IndexOrName::indexTag), "]"); > + return makeString(moduleName, ".wasm-function[", String(ion.m_indexName.name->data(), ion.m_indexName.name->size()), "]"); Nit: You could used a char ']' at the end instead of a "]" (may avoid a strlen if the compiler doesn't optimize it out). > Source/JavaScriptCore/wasm/WasmIndexOrName.h:46 > + NameSection* nameSection() const { return m_nameSection ? m_nameSection.get() : nullptr; } Style: This should just be `return m_nameSection.get()`, the ternary is unnecessary. > Source/JavaScriptCore/wasm/WasmNameSection.h:47 > + return std::make_pair(functionIndexSpace < functionNames.size() ? &functionNames[functionIndexSpace] : nullptr, RefPtr<NameSection>(this)); Style: `RefPtr<NameSection>(this)` is typically written as `makeRefPtr(this)`.
This looks to be a 1-2% WasmBench regression on some Macs.
My guess is SHA1 is hurting compile times.
I'd measured this to be neutral, and am surprised it would regress. I can't repro, tried a few things, so here's a speculative disable: https://bugs.webkit.org/show_bug.cgi?id=180441