[Webkit-unassigned] [Bug 228240] 3.5 MB system-wide footprint impact due to thread-locals in libANGLE

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=228240

--- Comment #6 from Dean Jackson <dino at apple.com> ---
Comment on attachment 434103
  --> https://bugs.webkit.org/attachment.cgi?id=434103
WIP

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

>> Source/ThirdParty/ANGLE/src/libANGLE/Context.cpp:302
>> +void FreeCurrentValidContext()
> 
> This and the function above are a little confusingly named. Could you consider:
> 
>   InitializeCurrentValidContextTLS
>   FreeCurrentValidContextTLS
> ?
> 
> I also noticed that FreeCurrentValidContext isn't called from anywhere - which is fine because it can really only be called upon unloading of ANGLE or application shutdown. Should a comment be added about when it could plausibly be used?

Done!

(The reason I prefixed the thread form with TLS was because there was already a method called GetCurrentThread()).

>> Source/ThirdParty/ANGLE/src/libGLESv2/global_state.cpp:47
>> +    static dispatch_once_t once = 0;
> 
> Better to leave off the = 0 and accept whatever the platform says is the default linker-initialized value.

Done

>>> Source/ThirdParty/ANGLE/src/libGLESv2/global_state.cpp:49
>>> +    if (!initialized) {
>> 
>> Is it necessary to do this double-checking? The documentation indicates that the correct usage is just to call dispatch_once unconditionally.
>> https://developer.apple.com/documentation/dispatch/1447169-dispatch_once
> 
> Kenneth is right. This is a bug. Because 'initialized' lacks any synchronization or memory barrier, it can read as true when it is still logically false.
> 
> I suggest removing the initialized variable and always calling dispatch_once. dispatch_once is fast (and inlined) in the "already dispatched" case.

Removed (although, question... even if it was read as true, the code would still hit the dispatch_once and sync there right?)

>> Source/ThirdParty/ANGLE/src/libGLESv2/global_state.cpp:99
>> +bool InitializeCurrentThreadTLS()
> 
> The naming convention between the CurrentThread and CurrentValidContext should match. So maybe here:
> 
> InitializeCurrentThreadTLSIndex
> FreeCurrentThreadTLSIndex
> GetCurrentThreadTLS
> SetCurrentThreadTLS
> 
> and use TLSIndex / TLS in the CurrentValidContext functions above.

Done.

>>> Source/ThirdParty/ANGLE/src/libGLESv2/global_state.cpp:119
>>> +    if (!initialized) {
>> 
>> Same question about the double-checked initialization, though I do see this is on the hot path.
> 
> Same comments as above.

Done. I also moved the initialization call into the Get/Set function to make it less confusing.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20210724/6a16b97c/attachment.htm>