WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
194824
Code quality cleanup in NeverDestroyed
https://bugs.webkit.org/show_bug.cgi?id=194824
Summary
Code quality cleanup in NeverDestroyed
Keith Miller
Reported
2019-02-19 11:55:45 PST
Code quality cleanup in NeverDestroyed
Attachments
Patch
(4.18 KB, patch)
2019-02-19 11:58 PST
,
Keith Miller
no flags
Details
Formatted Diff
Diff
Patch for landing
(4.01 KB, patch)
2019-02-19 12:08 PST
,
Keith Miller
no flags
Details
Formatted Diff
Diff
Patch for landing
(3.92 KB, patch)
2019-02-19 12:08 PST
,
Keith Miller
no flags
Details
Formatted Diff
Diff
Patch
(5.29 KB, patch)
2019-02-22 12:02 PST
,
Keith Miller
no flags
Details
Formatted Diff
Diff
Patch
(5.96 KB, patch)
2019-02-22 12:12 PST
,
Keith Miller
no flags
Details
Formatted Diff
Diff
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
Keith Miller
Comment 1
2019-02-19 11:58:11 PST
Created
attachment 362406
[details]
Patch
Yusuke Suzuki
Comment 2
2019-02-19 11:59:19 PST
Comment on
attachment 362406
[details]
Patch r=me
Keith Miller
Comment 3
2019-02-19 12:07:36 PST
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
Comment 4
2019-02-19 12:08:04 PST
Created
attachment 362409
[details]
Patch for landing
Keith Miller
Comment 5
2019-02-19 12:08:31 PST
Created
attachment 362410
[details]
Patch for landing
WebKit Commit Bot
Comment 6
2019-02-19 12:45:58 PST
Comment on
attachment 362410
[details]
Patch for landing Clearing flags on attachment: 362410 Committed
r241770
: <
https://trac.webkit.org/changeset/241770
>
WebKit Commit Bot
Comment 7
2019-02-19 12:45:59 PST
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 8
2019-02-19 12:52:47 PST
<
rdar://problem/48211208
>
Simon Fraser (smfr)
Comment 9
2019-02-19 15:09:24 PST
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
Comment 10
2019-02-22 12:02:14 PST
Created
attachment 362740
[details]
Patch
Keith Miller
Comment 11
2019-02-22 12:11:21 PST
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
Comment 12
2019-02-22 12:12:03 PST
Created
attachment 362741
[details]
Patch
Mark Lam
Comment 13
2019-02-26 16:31:44 PST
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 14
2019-02-26 16:33:43 PST
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 15
2019-02-26 16:59:55 PST
Comment on
attachment 362741
[details]
Patch Clearing flags on attachment: 362741 Committed
r242116
: <
https://trac.webkit.org/changeset/242116
>
WebKit Commit Bot
Comment 16
2019-02-26 16:59:57 PST
All reviewed patches have been landed. Closing bug.
Darin Adler
Comment 17
2019-03-03 00:45:30 PST
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
Comment 18
2019-03-03 11:48:52 PST
(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
Comment 19
2019-03-03 11:58:21 PST
(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.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug