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!
<rdar://problem/36474351>
Created attachment 331214 [details] patch
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.
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.
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.
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
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.
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.
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?
Created attachment 331239 [details] patch Implement suggestions.
Comment on attachment 331239 [details] patch Clearing flags on attachment: 331239 Committed r226920: <https://trac.webkit.org/changeset/226920>
All reviewed patches have been landed. Closing bug.