[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

Kenneth Russell <kbr at google.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #434103|review?                     |review+
              Flags|                            |

--- Comment #4 from Kenneth Russell <kbr at google.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

Looks good overall especially if this significantly reduces binary size, though it's unfortunate we can't use the built-in mechanisms.

A few questions / comments, but to unblock this work, r+ now.

> 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?

> 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

> 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.

> 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.

-- 
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/20210723/7965b5de/attachment.htm>