Code quality cleanup in NeverDestroyed
Created attachment 362406 [details] Patch
Comment on attachment 362406 [details] Patch r=me
Comment on attachment 362406 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362406&action=review > Source/WTF/wtf/NeverDestroyed.h:137 > - // LazyNeverDestroyed objects are always static, so this variable is initialized to false. > - // It must not be initialized dynamically; that would not be thread safe. > - bool m_isConstructed; > + bool m_isConstructed { false }; Nvm, this triggers the global constructor error to fire. We can't do this.
Created attachment 362409 [details] Patch for landing
Created attachment 362410 [details] Patch for landing
Comment on attachment 362410 [details] Patch for landing Clearing flags on attachment: 362410 Committed r241770: <https://trac.webkit.org/changeset/241770>
All reviewed patches have been landed. Closing bug.
<rdar://problem/48211208>
This caused a bunch of crashes: https://build.webkit.org/results/Apple%20High%20Sierra%20Debug%20WK1%20(Tests)/r241770%20(7858)/results.html Rolling out
Created attachment 362740 [details] Patch
Turns out the bug was that make_names.pl was just reinterpret_casting LazyNeverDestroyed to the type it holds... Not ideal. So I fixed that.
Created attachment 362741 [details] Patch
Comment on attachment 362741 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362741&action=review r=me > Source/WTF/wtf/NeverDestroyed.h:78 > + // FIXME: Investigate whether we should allocate a hunk of virtual memory /hunk/chunk/? > Source/WTF/wtf/NeverDestroyed.h:142 > + // FIXME: Investigate whether we should allocate a hunk of virtual memory /hunk/chunk/?
Comment on attachment 362741 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362741&action=review >> Source/WTF/wtf/NeverDestroyed.h:142 >> + // FIXME: Investigate whether we should allocate a hunk of virtual memory > > /hunk/chunk/? Dunno, I just moved it.
Comment on attachment 362741 [details] Patch Clearing flags on attachment: 362741 Committed r242116: <https://trac.webkit.org/changeset/242116>
Comment on attachment 362741 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362741&action=review > Source/WebCore/dom/make_names.pl:776 > - print F " reinterpret_cast<const WebCore::$parameters{namespace}QualifiedName*>(&${name}Tag),\n"; > + print F " &${name}Tag.get(),\n"; I think this was done so that the tables would be generated as read-only data rather than a long sequence of inlined code that initializes the tables. I suspect this change removes that optimization. Could you check? I know it might be inelegant, but I believe it made a substantial difference in code size.
(In reply to Darin Adler from comment #17) > Comment on attachment 362741 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=362741&action=review > > > Source/WebCore/dom/make_names.pl:776 > > - print F " reinterpret_cast<const WebCore::$parameters{namespace}QualifiedName*>(&${name}Tag),\n"; > > + print F " &${name}Tag.get(),\n"; > > I think this was done so that the tables would be generated as read-only > data rather than a long sequence of inlined code that initializes the > tables. I suspect this change removes that optimization. Could you check? I > know it might be inelegant, but I believe it made a substantial difference > in code size. Ah that’s a fair point there should probably be a comment about that. Is there any reason the tables can’t be a NeverDestroyed<std::array<>>? That seems like it would solve both problems.
(In reply to Keith Miller from comment #18) > Is there any reason the tables can’t be a NeverDestroyed<std::array<>>? > That seems like it would solve both problems. I don’t understand how std::array would help. The issue is whether this ends being data fixed up by the linker with no code, or code run the first time the function is run. Maybe std::array will help.