[webkit-reviews] review granted: [Bug 200329] [JSC] Support WebAssembly in SamplingProfiler : [Attachment 375328] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Yusuke Suzuki <ysuzuki at apple.com>'s
request for review:
Bug 200329: [JSC] Support WebAssembly in SamplingProfiler
https://bugs.webkit.org/show_bug.cgi?id=200329

Attachment 375328: Patch

https://bugs.webkit.org/attachment.cgi?id=375328&action=review




--- Comment #13 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 375328
  --> https://bugs.webkit.org/attachment.cgi?id=375328
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=375328&action=review

r=me

> Source/JavaScriptCore/runtime/SamplingProfiler.cpp:132
> +	   if (unsafeCallee.isWasm()) {

nit: Maybe we should name this function we're in "recordJITFrame" instead of
"recordJSFrame" since it now handles Wasm

> Source/JavaScriptCore/runtime/SamplingProfiler.cpp:135
> +		   // At this point, Wasm::Callee would be dying (ref count is
0), but its fields are still live.

this is the other end of the memory ordering I was talking about in ~Callee()

> Source/JavaScriptCore/runtime/SamplingProfiler.cpp:136
> +		   // And we can safely copy Wasm::IndexOrName even any lock is
held by suspended threads.

"even any" => "even when any"

> Source/JavaScriptCore/runtime/SamplingProfiler.h:71
> +	   Optional<Wasm::CompilationMode> wasmCompilationMode;

why not also put this under "#if ENABLE(WEBASSEMBLY)"?

(If it makes the code more annoying, I'd just skip doing it)

> Source/JavaScriptCore/runtime/SamplingProfiler.h:98
> +	   Optional<Wasm::CompilationMode> wasmCompilationMode;

ditto

> Source/JavaScriptCore/wasm/WasmCallee.cpp:52
> +    CalleeRegistry::singleton().unregisterCallee(this);

To be super pedantic, we may need a store store memory fence after this, since
theoretically, locking doesn't prevent stores below the lock is released into
moving into the lock region (just stores in the lock region can't move out).

So theoretically, the compiler could move stores of the fields of |this| object
before the store to remove the entry in the hash table.

Maybe we don't care about this memory ordering weirdness, but I think it might
be worth making sure we don't crash if those bits are bad.

> Source/JavaScriptCore/wasm/WasmCallee.h:64
> +	   return std::make_tuple(start, end);

style nit: I'd just do "return { start, end };"

> Source/JavaScriptCore/wasm/WasmCalleeRegistry.cpp:43
> +void CalleeRegistry::initialize()
> +{
> +    static std::once_flag onceKey;
> +    std::call_once(onceKey, [] {
> +	   calleeRegistry.construct();
> +    });
> +}

Since this is called from initializeThreading, you don't need this
synchronization to happen.

Alternatively, you could just not call this from initializeThreading and keep
things this way.