Bug 47608 - Add !SINGLE_THREADED guard.
Summary: Add !SINGLE_THREADED guard.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-10-13 11:36 PDT by Hyung Song
Modified: 2010-10-17 17:04 PDT (History)
3 users (show)

See Also:


Attachments
Patch. Add !SINGLE_THREADED guard. (1.21 KB, patch)
2010-10-13 11:38 PDT, Hyung Song
levin: review-
Details | Formatted Diff | Diff
patch (1.02 KB, patch)
2010-10-14 17:30 PDT, Hyung Song
levin: review-
Details | Formatted Diff | Diff
patch (1.29 KB, patch)
2010-10-14 17:48 PDT, Hyung Song
ap: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
patch (1.29 KB, patch)
2010-10-15 09:49 PDT, Hyung Song
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hyung Song 2010-10-13 11:36:35 PDT
Single-threaded ports do not have mutex or yield.
Comment 1 Hyung Song 2010-10-13 11:38:54 PDT
Created attachment 70639 [details]
Patch. Add !SINGLE_THREADED guard.
Comment 2 David Levin 2010-10-13 22:44:50 PDT
Comment on attachment 70639 [details]
Patch. Add !SINGLE_THREADED guard.

It seems like it would be better to implement a mutex whose tryLock returns true rather than putting these if'defs throughout the code.

You could also have a "yield" that does nothing.
Comment 3 Alexey Proskuryakov 2010-10-14 10:52:54 PDT
There is already ThreadingNone.cpp - and since it's behind ENABLE(SINGLE_THREADED), I expect it to be present in your port. So, the explanation in ChangeLog is misleading.

The reason tryLock() in ThreadingNone.cpp returns false is that we have lots of assertions of this form: ASSERT(!m_databaseGuard.tryLock()).

So, even while yield() can just be added to ThreadingNone.cpp, changing tryLock() to support the logic in this function is more involved. Perhaps the approach in this patch is the right one. David, what do you think?
Comment 4 David Levin 2010-10-14 13:40:30 PDT
(In reply to comment #3)
> There is already ThreadingNone.cpp - and since it's behind ENABLE(SINGLE_THREADED), I expect it to be present in your port. So, the explanation in ChangeLog is misleading.
> 
> The reason tryLock() in ThreadingNone.cpp returns false is that we have lots of assertions of this form: ASSERT(!m_databaseGuard.tryLock()).
> 
> So, even while yield() can just be added to ThreadingNone.cpp, changing tryLock() to support the logic in this function is more involved. Perhaps the approach in this patch is the right one. David, what do you think?

Sounds reasonable.

Basically, the simplest thing is to fix the ChangeLog, and that would be sufficent.


As a thought exercise, I wonder if the mutex would work PlatformMutex was "bool m_lockTaken".

void Mutex::lock() { m_lockTaken = true; }
bool Mutex::tryLock() 
{ 
    if (!m_lockTaken) 
        return true; 
    return false;
}
void Mutex::unlock() { m_lockTaken = false; }

Of course this means that there is an extra bool of memory consumed (and I little more code compiled). I don't know how much that is a factor.
Comment 5 Hyung Song 2010-10-14 17:30:10 PDT
Created attachment 70807 [details]
patch

Add dummy yield() for SINGLE_THREADED.
Comment 6 David Levin 2010-10-14 17:43:48 PDT
(In reply to comment #5)
> Created an attachment (id=70807) [details]
> patch
> 
> Add dummy yield() for SINGLE_THREADED.

Alexey pointed out that this won't work for you.

You either need to go with the old patch and fix the ChangeLog or fix the mutex class for this case

I think the following would work, but you'd need to try it out:

In JavaScriptCore/wtf/ThreadingPrimitives.h

Change 
   typedef void* PlatformMutex;
to
   typedef bool PlatformMutex;

// Use m_mutex to indicate if the mutex is "held" or not.
Mutex::Mutex() : m_mutex(false) { }
void Mutex::lock() { m_mutex = true; }
bool Mutex::tryLock() 
{ 
    if (m_mutex)
        return false;
    m_mutex = true;
    return true; 
}
void Mutex::unlock() { m_mutex = false; }
Comment 7 David Levin 2010-10-14 17:44:13 PDT
Comment on attachment 70807 [details]
patch

As noted above.
Comment 8 Hyung Song 2010-10-14 17:48:59 PDT
Created attachment 70811 [details]
patch

Changed ChangeLog.
Comment 9 Alexey Proskuryakov 2010-10-14 22:00:09 PDT
Comment on attachment 70811 [details]
patch

+        For SINGLE_THREADED ports LockingMutex.tryLock() returns false.
+        This will make interrupt() fall into infinite loop.

A slightly better way to say this would be "This was making interrupt() fall into infinite loop". The current text sounds like your patch introduces falling into infinite loop. But it's OK.
Comment 10 WebKit Commit Bot 2010-10-14 22:01:26 PDT
Comment on attachment 70811 [details]
patch

Rejecting patch 70811 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=abarth-cq-sl', 'apply-attachment', '--force-clean', '--non-interactive', '--quiet', 70811]" exit_code: 2
Cleaning working directory
Updating working directory
Logging in as commit-queue@webkit.org...
Fetching: https://bugs.webkit.org/attachment.cgi?id=70811&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=47608&ctype=xml
Processing 1 patch from 1 bug.
Processing patch 70811 from bug 47608.
Failed to run "[u'/Users/abarth/git/webkit-queue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Alexey Proskuryakov', u'--force']" exit_code: 2

Full output: http://queues.webkit.org/results/4408033
Comment 11 David Levin 2010-10-14 22:45:56 PDT
The commit queue didn't work because the patch is messed up in the ChangeLog portion. From the style bot:

Failed to run "[u'/mnt/git/webkit-chromium-ews/WebKitTools/Scripts/svn-apply', u'--force']" exit_code: 2
Parsed 2 diffs from patch file(s).
patching file WebCore/ChangeLog
patch: **** malformed patch at line 21:          Reviewed by Steve Block.

Hyung, you are best off fixing the patch in my opinion.
Comment 12 Hyung Song 2010-10-15 09:49:50 PDT
Created attachment 70878 [details]
patch

Fix ChangeLog conflict, and change ChangeLog to explain better.
Comment 13 David Levin 2010-10-17 16:50:42 PDT
Comment on attachment 70878 [details]
patch

ok
Comment 14 WebKit Commit Bot 2010-10-17 17:04:36 PDT
Comment on attachment 70878 [details]
patch

Clearing flags on attachment: 70878

Committed r69935: <http://trac.webkit.org/changeset/69935>
Comment 15 WebKit Commit Bot 2010-10-17 17:04:42 PDT
All reviewed patches have been landed.  Closing bug.