Bug 181599 – PoisonedWriteBarrier

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[JF Bastien]

Allow poisoning of WriteBarrier objects, and use this in a place where it should be perf-neutral first. If it indeed is perf-neutral, start using it in more risky places!

[Radar WebKit Bug Importer]

<rdar://problem/36474351>

[JF Bastien]

Created attachment 331214 [details]
patch

[EWS Watchlist]

Attachment 331214 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/WriteBarrier.h:33:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

Comment on attachment 331214 [details]
patch

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

Please make sure the EWS bots are happy.  I'll wait for you to rebase before continuing the review.  It'll make it easier to do the review.  Thanks.

> Source/JavaScriptCore/ChangeLog:12
> +        using it in more risky places!

I suggest rephrasing /in more risky places!/in more potentially performance sensitive places./

> Source/JavaScriptCore/runtime/WriteBarrier.h:34
> +#include <type_traits>

Please fix ordering.

> Source/WTF/wtf/Poisoned.h:92
> +template<typename KeyType, KeyType Key, typename T, typename = std::enable_if_t<std::is_pointer<T>::value>>

Why uppercase Key?  Isn't our convention to use lowercase for variables / arguments, and uppercase for Types?  I suggest reverting this here and below as well.

[JF Bastien]

Created attachment 331219 [details]
patch

Address comments.

This should also fix the build (still going on my laptop...): code was indirectly doing WriteBarrier<>.set() on JSGlobalObject with only a forward declaration, so it didn't know that JSGlobalObject derives from JSCell. This is now a compilation error because I static_cast to JSCell*, instead of reinterpret_cast. This particular case was fine because the object layout didn't change, but in general static_cast might require a base adjustment. It's also good to check that there indeed is an inheritance hierarchy! I fixed the issue by moving Structure.h's uses of .set() to its Inlines.h file.

[JF Bastien]

Created attachment 331232 [details]
js benchmarks

In addition to WasmBench which I ran earlier, I also ran our JS benchmarks to confirm that the WriteBarrier changes had no effect. They're indeed totally neutral (results attached).


WasmBench results (with 10 iterations) where:

Compile score (as arithmetic mean of each compile iteration)  90.30 +- 1.45 ms
Throughput score (as arithmetic mean of each throughput iteration)  3359.70 +- 22.63 ms
Total score (as arithmetic mean of each score iteration)  550.76 +- 5.15 ms

Compile score (as arithmetic mean of each compile iteration)  90.62 +- 1.23 ms
Throughput score (as arithmetic mean of each throughput iteration)  3387.59 +- 16.70 ms
Total score (as arithmetic mean of each score iteration)  554.03 +- 3.60 ms

[Mark Lam]

Comment on attachment 331219 [details]
patch

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

I think you have a bug.  Please fix.

> Source/JavaScriptCore/runtime/WriteBarrier.h:113
> +        validateCell<T>(Traits::unwrap(cell));
> +        return Traits::unwrap(cell);

I suggest pre-caching the unwrapped pointer:
    T* resultCell = Traits::unwrap(cell);
    validateCell<T>(resultCell);
    return resultCell;

> Source/JavaScriptCore/runtime/WriteBarrier.h:121
> +        validateCell(Traits::unwrap(cell));
> +        return Traits::unwrap(cell);

Ditto.  precache?

> Source/JavaScriptCore/wasm/js/WasmToJS.cpp:482
> +    jit.move(CCallHelpers::TrustedImm64(JSWebAssemblyInstance::PoisonedBarrier<WebAssemblyToJSCallee>::poison), importJSCellGPRReg);

It is wrong to assume that importJSCellGPRReg (i.e. GPRInfo::regT0) !== GPRInfo::argumentGPR0.  In fact, on ARM64 (and most other architectures), they are the same.  You're just lucky that they aren't the same on x86_64 here.  Please fix.

[JF Bastien]

Created attachment 331235 [details]
patch

> > Source/JavaScriptCore/runtime/WriteBarrier.h:113
> > +        validateCell<T>(Traits::unwrap(cell));
> > +        return Traits::unwrap(cell);
> 
> I suggest pre-caching the unwrapped pointer:
>     T* resultCell = Traits::unwrap(cell);
>     validateCell<T>(resultCell);
>     return resultCell;
> 
> > Source/JavaScriptCore/runtime/WriteBarrier.h:121
> > +        validateCell(Traits::unwrap(cell));
> > +        return Traits::unwrap(cell);
> 
> Ditto.  precache?

Done x 2.

> > Source/JavaScriptCore/wasm/js/WasmToJS.cpp:482
> > +    jit.move(CCallHelpers::TrustedImm64(JSWebAssemblyInstance::PoisonedBarrier<WebAssemblyToJSCallee>::poison), importJSCellGPRReg);
> 
> It is wrong to assume that importJSCellGPRReg (i.e. GPRInfo::regT0) !==
> GPRInfo::argumentGPR0.  In fact, on ARM64 (and most other architectures),
> they are the same.  You're just lucky that they aren't the same on x86_64
> here.  Please fix.

Nice, good catch! I hadn't looked at all the ISAs. Fixed now.

[Mark Lam]

Comment on attachment 331235 [details]
patch

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

r=me with suggestion.  Please make sure the EWS bots are green before landing.  Thanks.

> Source/JavaScriptCore/wasm/js/WasmToJS.cpp:56
> -static void materializeImportJSCell(JIT& jit, unsigned importIndex, GPRReg result)
> +static void materializeImportJSCell(JIT& jit, unsigned importIndex, GPRReg scratch, GPRReg result)
>  {
>      // We're calling out of the current WebAssembly.Instance. That Instance has a list of all its import functions.
> -    jit.loadWasmContextInstance(result);
> -    jit.loadPtr(JIT::Address(result, Instance::offsetOfImportFunction(importIndex)), result);
> +    jit.loadWasmContextInstance(scratch);
> +    jit.loadPtr(JIT::Address(scratch, Instance::offsetOfImportFunction(importIndex)), scratch);
> +    jit.move(CCallHelpers::TrustedImm64(JSWebAssemblyInstance::PoisonedBarrier<JSObject>::poison), result);
> +    jit.xor64(scratch, result);
>  }

In both clients that calls materializeImportJSCell, they already loaded JSWebAssemblyInstance::PoisonedBarrier<JSObject>::poison into scratch.  Can we simplify this as:

static void materializeImportJSCell(JIT& jit, unsigned importIndex, GPRReg poison, GPRReg result)
{
    // We're calling out of the current WebAssembly.Instance. That Instance has a list of all its import functions.
    jit.loadWasmContextInstance(result);
    jit.loadPtr(JIT::Address(result, Instance::offsetOfImportFunction(importIndex)), result);
    jit.xor64(poison, result);
}

> Source/JavaScriptCore/wasm/js/WasmToJS.cpp:295
> +        jit.move(CCallHelpers::TrustedImm64(JSWebAssemblyInstance::PoisonedBarrier<WebAssemblyToJSCallee>::poison), GPRInfo::argumentGPR1);

Jus move the poison into argumentGPR2 as a poisonScratch so that you can skip doing it in materializeImportJSCell below.

> Source/JavaScriptCore/wasm/js/WasmToJS.cpp:299
> +        materializeImportJSCell(jit, importIndex, GPRInfo::argumentGPR2, GPRInfo::argumentGPR1);

Do you need to clear the poison after this as well before the call?

[JF Bastien]

Created attachment 331239 [details]
patch

Implement suggestions.

[WebKit Commit Bot]

Comment on attachment 331239 [details]
patch

Clearing flags on attachment: 331239

Committed r226920: <https://trac.webkit.org/changeset/226920>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.