[webkit-reviews] review granted: [Bug 227757] Fix uses of Dependency::fence with respect to the compiler outsmarting us : [Attachment 433981] patch

[bugzilla-daemon at webkit.org]

Robin Morisset <rmorisset at apple.com> has granted Saam Barati
<sbarati at apple.com>'s request for review:
Bug 227757: Fix uses of Dependency::fence with respect to the compiler
outsmarting us
https://bugs.webkit.org/show_bug.cgi?id=227757

Attachment 433981: patch

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




--- Comment #11 from Robin Morisset <rmorisset at apple.com> ---
Comment on attachment 433981
  --> https://bugs.webkit.org/attachment.cgi?id=433981
patch

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

r=me

> Source/JavaScriptCore/runtime/SparseArrayValueMap.cpp:-170
> -JSValue SparseArrayEntry::getConcurrently() const

I'm a bit surprised that you had to remove const here.
Could this be fixed by also having a version of loadAndFence that takes a const
T* ?

>> Source/WTF/wtf/Atomics.h:439
>> +	static Dependency loadAndFence(T* pointer, T& output)
> 
> I could also change the API to be pair based instead of using an out
parameter.

It would be cleaner, but considering that this is going to be on very hot
paths, and my lack of trust in LLVM's abilitiy to optimize a struct return
type, I'm in favor of sticking with the out param.