Bug 185349 – [WTF] Embed default atomic string table in Thread

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

[Yusuke Suzuki]

[WTF] Embed default atomic string table in Thread

[Yusuke Suzuki]

Created attachment 339640 [details]
Patch

[Yusuke Suzuki]

Created attachment 339643 [details]
Patch

[Yusuke Suzuki]

Created attachment 339644 [details]
Patch

[Darin Adler]

Comment on attachment 339644 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=339644&action=review

I’m not going to say r=me yet because I am not sure this is OK for iOS

> Source/WTF/ChangeLog:9
> +        Do not need to allocate AtomicStringTable separately from Thread
> +        since the table's lifetime is completely the same to Thread.

"separately from Thread" -> "separate from Thread"
"same to Thread" -> "same as Thread"

> Source/WTF/ChangeLog:11
> +        This patch just embeds it into Thread's field.

Better grammar and standard C++ terminology: instead of "embeds it into Thread's field", say "embeds it as a data member of Thread".

> Source/WTF/wtf/Threading.cpp:109
> +#if USE(WEB_THREAD)
> +    // On iOS, one AtomicStringTable is shared between the main UI thread and the WebThread.
> +    static AtomicStringTable* sharedStringTable = new AtomicStringTable;
> +
> +    if (isWebThread() || isUIThread())
> +        m_currentAtomicStringTable = sharedStringTable;
> +#endif // USE(WEB_THREAD)

I think it’s easier to read if we put the sharedStringTable static inside the if statement. Also, we could use a NeverDestroyed instead of putting it on the heap. Why not do that?

Doesn’t this mean we will have two initialized, but unused, empty AtomicStringTable instances on iOS, one in the web thread’s Thread object and one in the  UI thread’s Thread object? Is that waste OK because AtomicStringTable is a small object?

When the block of code is this small, I don’t think it needs to have a comment on the #endif.

[Yusuke Suzuki]

Comment on attachment 339644 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=339644&action=review

>> Source/WTF/ChangeLog:9
>> +        since the table's lifetime is completely the same to Thread.
> 
> "separately from Thread" -> "separate from Thread"
> "same to Thread" -> "same as Thread"

Fixed.

>> Source/WTF/ChangeLog:11
>> +        This patch just embeds it into Thread's field.
> 
> Better grammar and standard C++ terminology: instead of "embeds it into Thread's field", say "embeds it as a data member of Thread".

Fixed, thanks.

>> Source/WTF/wtf/Threading.cpp:109
>> +#endif // USE(WEB_THREAD)
> 
> I think it’s easier to read if we put the sharedStringTable static inside the if statement. Also, we could use a NeverDestroyed instead of putting it on the heap. Why not do that?
> 
> Doesn’t this mean we will have two initialized, but unused, empty AtomicStringTable instances on iOS, one in the web thread’s Thread object and one in the  UI thread’s Thread object? Is that waste OK because AtomicStringTable is a small object?
> 
> When the block of code is this small, I don’t think it needs to have a comment on the #endif.

I've changed this to use NeverDestroyed<> and moved it inside if-statement.

Yeah, on main and web threads, we have unused AtomicStringTable. And it is OK since (1) only main and web threads (2 threads) discard it and (2) empty AtomicStringTable is very small (2 pointer size, they are just 0 initialized, no external allocation).
This patch does not break the current iOS web thread semantics. m_defaultAtomicStringTable is not touched except for here. And web thread never exits.

And removed #endif's comment.

[Yusuke Suzuki]

Created attachment 339702 [details]
Patch

[Yusuke Suzuki]

Committed r231404: <https://trac.webkit.org/changeset/231404>

[Radar WebKit Bug Importer]

<rdar://problem/40015023>