Bug 108139 - Upstream iOS's AtomicString
Summary: Upstream iOS's AtomicString
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Benjamin Poulain
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-01-28 17:34 PST by Benjamin Poulain
Modified: 2013-02-04 14:22 PST (History)
5 users (show)

See Also:


Attachments
Patch (5.68 KB, patch)
2013-01-28 18:12 PST, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch (6.47 KB, patch)
2013-01-29 16:39 PST, Benjamin Poulain
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Poulain 2013-01-28 17:34:17 PST
Some more upstreaming.
Comment 1 Benjamin Poulain 2013-01-28 18:12:25 PST
Created attachment 185123 [details]
Patch
Comment 2 Sam Weinig 2013-01-28 19:23:02 PST
Comment on attachment 185123 [details]
Patch

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

> Source/WTF/wtf/text/AtomicString.cpp:34
> +#include <libkern/OSAtomic.h>

Can we use TCSpinLock instead?

> Source/WTF/wtf/text/AtomicString.cpp:44
> +#if PLATFORM(IOS)

I would like this to be a check for something more specific.  Maybe USE(WEB_THREAD)

> Source/WTF/wtf/text/AtomicString.cpp:121
> +#if PLATFORM(IOS)
> +    AtomicStringTableLocker locker;
> +#endif

Instead of having all these #ifdefs at the call site, could we just make those without a WebThread have an empty implementation of AtomicStringTableLocker?
Comment 3 Benjamin Poulain 2013-01-28 20:04:38 PST
Comment on attachment 185123 [details]
Patch

> > Source/WTF/wtf/text/AtomicString.cpp:44
> > +#if PLATFORM(IOS)
> 
> I would like this to be a check for something more specific.  Maybe USE(WEB_THREAD)

I like that idea.

> > Source/WTF/wtf/text/AtomicString.cpp:121
> > +#if PLATFORM(IOS)
> > +    AtomicStringTableLocker locker;
> > +#endif
> 
> Instead of having all these #ifdefs at the call site, could we just make those without a WebThread have an empty implementation of AtomicStringTableLocker?

I am afraid someone will misunderstand what it is and use AtomicStringTableLocker at a bad place. With the #ifdef, it is clear you should not touch that if not for iOS.
Comment 4 Adam Barth 2013-01-28 21:25:25 PST
Comment on attachment 185123 [details]
Patch

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

> Source/WTF/ChangeLog:9
> +        On iOS, WebCore can be executed from two different threads. To maintain consistency,
> +        a few changes had been made:

How many changes are required in WebCore to deal with this issue?
Comment 5 Benjamin Poulain 2013-01-28 22:00:58 PST
> > Source/WTF/ChangeLog:9
> > +        On iOS, WebCore can be executed from two different threads. To maintain consistency,
> > +        a few changes had been made:
> 
> How many changes are required in WebCore to deal with this issue?

Not much in common code. It requires quite a bit of code in the iOS platform code. The code does not become multithreaded in any way, the two threads are exclusive.

A lot of assertions (ASSERT(isMainThread())) need to be updated but that change is completely transparent if you don't have a WebThread.
Comment 6 Benjamin Poulain 2013-01-29 16:39:09 PST
Created attachment 185340 [details]
Patch
Comment 7 David Kilzer (:ddkilzer) 2013-01-30 04:38:46 PST
Comment on attachment 185340 [details]
Patch

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

> Source/WTF/wtf/Platform.h:620
> +#define WTF_USE_WEB_THREAD 1

What about USE(IOS_WEB_THREAD) instead?
Comment 8 Benjamin Poulain 2013-02-04 12:48:25 PST
> > Source/WTF/wtf/Platform.h:620
> > +#define WTF_USE_WEB_THREAD 1
> 
> What about USE(IOS_WEB_THREAD) instead?

I think having IOS in a USE() macro defeats the purpose a bit. But maybe Web_Thread is ambiguous?
Maybe?:
USE(WEBCORE_WITH_THREADS)
USE(THREADED_ACCESSS)
USE(MULTITHREAD_WEBKIT)

I don't mind in any way. I can update the macro before landing.
Comment 9 David Kilzer (:ddkilzer) 2013-02-04 13:21:18 PST
Comment on attachment 185340 [details]
Patch

r=me
Comment 10 Benjamin Poulain 2013-02-04 14:22:11 PST
Comment on attachment 185340 [details]
Patch

Clearing flags on attachment: 185340

Committed r141812: <http://trac.webkit.org/changeset/141812>
Comment 11 Benjamin Poulain 2013-02-04 14:22:13 PST
All reviewed patches have been landed.  Closing bug.