Bug 194824 – Code quality cleanup in NeverDestroyed

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Keith Miller]

Code quality cleanup in NeverDestroyed

[Keith Miller]

Created attachment 362406 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 362406 [details]
Patch

r=me

[Keith Miller]

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.

[Keith Miller]

Created attachment 362409 [details]
Patch for landing

[Keith Miller]

Created attachment 362410 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 362410 [details]
Patch for landing

Clearing flags on attachment: 362410

Committed r241770: <https://trac.webkit.org/changeset/241770>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/48211208>

[Simon Fraser (smfr)]

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

[Keith Miller]

Created attachment 362740 [details]
Patch

[Keith Miller]

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.

[Keith Miller]

Created attachment 362741 [details]
Patch

[Mark Lam]

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

[Keith Miller]

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.

[WebKit Commit Bot]

Comment on attachment 362741 [details]
Patch

Clearing flags on attachment: 362741

Committed r242116: <https://trac.webkit.org/changeset/242116>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

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.

[Keith Miller]

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

[Darin Adler]

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