Bug 47608 – Add !SINGLE_THREADED guard.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Hyung Song]

Single-threaded ports do not have mutex or yield.

[Hyung Song]

Created attachment 70639 [details]
Patch. Add !SINGLE_THREADED guard.

[David Levin]

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.

[Alexey Proskuryakov]

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?

[David Levin]

(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.

[Hyung Song]

Created attachment 70807 [details]
patch

Add dummy yield() for SINGLE_THREADED.

[David Levin]

(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; }

[David Levin]

Comment on attachment 70807 [details]
patch

As noted above.

[Hyung Song]

Created attachment 70811 [details]
patch

Changed ChangeLog.

[Alexey Proskuryakov]

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.

[WebKit Commit Bot]

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 [e-mail]...
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

[David Levin]

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.

[Hyung Song]

Created attachment 70878 [details]
patch

Fix ChangeLog conflict, and change ChangeLog to explain better.

[David Levin]

Comment on attachment 70878 [details]
patch

ok

[WebKit Commit Bot]

Comment on attachment 70878 [details]
patch

Clearing flags on attachment: 70878

Committed r69935: <http://trac.webkit.org/changeset/69935>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.