[webkit-reviews] review granted: [Bug 228240] 3.5 MB system-wide footprint impact due to thread-locals in libANGLE : [Attachment 434103] WIP
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Jul 23 12:02:35 PDT 2021
Kenneth Russell <kbr at google.com> has granted Dean Jackson <dino at apple.com>'s
request for review:
Bug 228240: 3.5 MB system-wide footprint impact due to thread-locals in
libANGLE
https://bugs.webkit.org/show_bug.cgi?id=228240
Attachment 434103: WIP
https://bugs.webkit.org/attachment.cgi?id=434103&action=review
--- 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.
More information about the webkit-reviews
mailing list