Bug 91746 - set WTF_USE_LOCKFREE_THREADSAFEREFCOUNTED for chromium android
Summary: set WTF_USE_LOCKFREE_THREADSAFEREFCOUNTED for chromium android
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Android Android
: P2 Normal
Assignee: Wei James (wistoch)
URL:
Keywords:
Depends on: 92635
Blocks: 89428
  Show dependency treegraph
 
Reported: 2012-07-19 08:35 PDT by Wei James (wistoch)
Modified: 2012-07-30 16:54 PDT (History)
4 users (show)

See Also:


Attachments
Patch (1016 bytes, patch)
2012-07-19 08:35 PDT, Wei James (wistoch)
no flags Details | Formatted Diff | Diff
Patch (1.63 KB, patch)
2012-07-26 00:55 PDT, Wei James (wistoch)
no flags Details | Formatted Diff | Diff
Patch (1.63 KB, patch)
2012-07-26 00:58 PDT, Wei James (wistoch)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Wei James (wistoch) 2012-07-19 08:35:04 PDT
set WTF_USE_LOCKFREE_THREADSAFEREFCOUNTED for chromium android
Comment 1 Wei James (wistoch) 2012-07-19 08:35:24 PDT
Created attachment 153271 [details]
Patch
Comment 2 Adam Barth 2012-07-25 23:22:17 PDT
Comment on attachment 153271 [details]
Patch

Why?
Comment 3 Wei James (wistoch) 2012-07-25 23:35:15 PDT
(In reply to comment #2)
> (From update of attachment 153271 [details])
> Why?

it is part of the efforts to enable web audio for chromium android. 

as mentioned in https://bugs.webkit.org/show_bug.cgi?id=89428#c19

peter wants to seperate the patch to two pieces. the one in #89428 already landed webkit. but I cannot find digit@google.com on bugs.webkit.org so have to turn to you. :)
Comment 4 Adam Barth 2012-07-26 00:02:02 PDT
Would you be willing to include this information in the ChangeLog?  As written the patch is very mysterious.
Comment 5 Adam Barth 2012-07-26 00:03:01 PDT
From the name, WTF_USE_LOCKFREE_THREADSAFEREFCOUNTED sounds like it could have much wider implications than just web audio.  Are we sure we want those implications?  Do we turn this on for Chromium on other operating systems?
Comment 6 Wei James (wistoch) 2012-07-26 00:25:07 PDT
(In reply to comment #4)
> Would you be willing to include this information in the ChangeLog?  As written the patch is very mysterious.

sure. I will update the patch to include this information. thanks
Comment 7 Wei James (wistoch) 2012-07-26 00:31:24 PDT
(In reply to comment #5)
> From the name, WTF_USE_LOCKFREE_THREADSAFEREFCOUNTED sounds like it could have much wider implications than just web audio.  Are we sure we want those implications?  Do we turn this on for Chromium on other operating systems?

I was confused too when I saw this MACRO at the begining. 

this MACRO is to eanble using WTF::atomicDecrement and WTF::atomicIncreament in wtf/Atomics.h

from wtf/Atomics.h, seems all other OS and platforms enabled this MACRO.
Comment 8 Wei James (wistoch) 2012-07-26 00:33:04 PDT
(In reply to comment #5)
> From the name, WTF_USE_LOCKFREE_THREADSAFEREFCOUNTED sounds like it could have much wider implications than just web audio.  Are we sure we want those implications?  Do we turn this on for Chromium on other operating systems?

it is not just for web audio. it is for wtf::atomicDecrement and wtf::atomicIncrement. 

Web Audio just needs to use atomicDecrement and atomicIncrement.
Comment 9 Wei James (wistoch) 2012-07-26 00:55:45 PDT
Created attachment 154566 [details]
Patch
Comment 10 Wei James (wistoch) 2012-07-26 00:58:12 PDT
Created attachment 154567 [details]
Patch
Comment 11 Wei James (wistoch) 2012-07-26 01:02:14 PDT
after reviewing the comments in #89428, it may make more sense to put this MACRO in Atomics.h. 

also, added more information in ChangeLog. thanks
Comment 12 Adam Barth 2012-07-26 01:54:33 PDT
Comment on attachment 154567 [details]
Patch

Thanks!  This patch makes much more sense.
Comment 13 Peter Beverloo 2012-07-26 01:55:55 PDT
I've asked David Turner to have a quick peek as well, as I vaguely recall issues with this. I can set CQ+ once we're good. Thank you, James Wei!
Comment 14 Wei James (wistoch) 2012-07-26 01:57:30 PDT
(In reply to comment #12)
> (From update of attachment 154567 [details])
> Thanks!  This patch makes much more sense.

thanks!
Comment 15 Wei James (wistoch) 2012-07-26 01:57:47 PDT
(In reply to comment #13)
> I've asked David Turner to have a quick peek as well, as I vaguely recall issues with this. I can set CQ+ once we're good. Thank you, James Wei!

that's fine. thanks!
Comment 16 Peter Beverloo 2012-07-27 06:14:42 PDT
The issues were fixed starting the release of r7b of the Android NDK, which is what WebKit uses. Setting cq+, thank you for the patch!
Comment 17 WebKit Review Bot 2012-07-27 07:15:07 PDT
Comment on attachment 154567 [details]
Patch

Clearing flags on attachment: 154567

Committed r123875: <http://trac.webkit.org/changeset/123875>
Comment 18 WebKit Review Bot 2012-07-27 07:15:11 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 Alexandre Elias 2012-07-30 01:03:48 PDT
This is causing FunctionalTest.MemberFunctionBindRefDeref in the TestWebKitAPI test package to fail on our private bot.
Comment 20 Alexandre Elias 2012-07-30 01:08:46 PDT
Not sure of the reason why it has issues on Android, given that the setting is enabled on many other platforms without problems.  Wei, could you investigate?
Comment 21 Alexandre Elias 2012-07-30 01:18:13 PDT
In webkit_unit_tests package, IDBDatabaseBackendTest.BackingStoreRetention is also failing with backingStore->hasOneRef() (Actual: false, Expected: true).
Comment 22 Wei James (wistoch) 2012-07-30 01:30:18 PDT
(In reply to comment #20)
> Not sure of the reason why it has issues on Android, given that the setting is enabled on many other platforms without problems.  Wei, could you investigate?

ok. I will investigate it.
Comment 23 Peter Beverloo 2012-07-30 02:11:52 PDT
I think it's unreasonable to ask Wei James to investigate issues occurring on our private branch. We should pick this up ourselves.

Alexandre, what version of the Android NDK do we use downstream? The atomic operations should be safe to use starting version r7b..
Comment 24 Wei James (wistoch) 2012-07-30 02:53:54 PDT
(In reply to comment #23)
> I think it's unreasonable to ask Wei James to investigate issues occurring on our private branch. We should pick this up ourselves.
> 
> Alexandre, what version of the Android NDK do we use downstream? The atomic operations should be safe to use starting version r7b..

peter, I can reproduce this issue in chromium upstream with ndk r8. 

it is strange as __atomic_inc is implemented with __sync_fetch_and_add in r8.
Comment 25 Wei James (wistoch) 2012-07-30 04:11:08 PDT
(In reply to comment #24)
> (In reply to comment #23)
> > I think it's unreasonable to ask Wei James to investigate issues occurring on our private branch. We should pick this up ourselves.
> > 
> > Alexandre, what version of the Android NDK do we use downstream? The atomic operations should be safe to use starting version r7b..
> 
> peter, I can reproduce this issue in chromium upstream with ndk r8. 
> 
> it is strange as __atomic_inc is implemented with __sync_fetch_and_add in r8.

I have found the root cause of this issue. 

__atomic_inc is safe to use in android. 

but in WebKit, it is assumed that __atomic_inc will return the new value. 

while in Android, __sync_fetch_and_add will return the old value. 

so ThreadSafeRefCounted.h, Line 108: 

should be atomicDecrement(&m_refCount) <= 1 in android instead of 
atomicDecrement(&m_refCount) <= 0

I will refine and submit the patch later for review. thanks
Comment 26 Peter Beverloo 2012-07-30 16:54:20 PDT
Marking this as fixed again.