Bug 41792 - [WINCE] Implement TCSpinLock
Summary: [WINCE] Implement TCSpinLock
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-07-07 12:59 PDT by Patrick R. Gansterer
Modified: 2010-07-23 09:24 PDT (History)
3 users (show)

See Also:


Attachments
Patch (2.15 KB, patch)
2010-07-07 13:01 PDT, Patrick R. Gansterer
aroben: review-
Details | Formatted Diff | Diff
Patch (2.14 KB, patch)
2010-07-23 08:36 PDT, Patrick R. Gansterer
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Patrick R. Gansterer 2010-07-07 12:59:16 PDT
InterlockedExchange is available on all windows platforms and can be in WinCE or as fallback:
http://msdn.microsoft.com/en-us/library/ms683590%28VS.85%29.aspx
Comment 1 Patrick R. Gansterer 2010-07-07 13:01:27 PDT
Created attachment 60768 [details]
Patch
Comment 2 WebKit Review Bot 2010-07-07 13:02:17 PDT
Attachment 60768 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
JavaScriptCore/wtf/TCSpinLock.h:198:  "windows.h" already included at JavaScriptCore/wtf/TCSpinLock.h:55  [build/include] [4]
JavaScriptCore/wtf/TCSpinLock.h:200:  TCMalloc_SlowLock is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/TCSpinLock.h:205:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
JavaScriptCore/wtf/TCSpinLock.h:210:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
JavaScriptCore/wtf/TCSpinLock.h:214:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
JavaScriptCore/wtf/TCSpinLock.h:215:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
JavaScriptCore/wtf/TCSpinLock.h:225:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
JavaScriptCore/wtf/TCSpinLock.h:225:  TCMalloc_SlowLock is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/TCSpinLock.h:226:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 9 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Adam Roben (:aroben) 2010-07-23 08:25:58 PDT
Comment on attachment 60768 [details]
Patch

> +#elif OS(WINDOWS)

I don't understand how this code will be compiled for WinCE. The first branch of this #if/#elif chain is:

#if (CPU(X86) || CPU(X86_64) || CPU(PPC)) && (COMPILER(GCC) || COMPILER(MSVC))

Will WinCE not match that condition?

> +    LONG m_lockword;

I think m_lockword is supposed to be a volatile LONG.


> +static void TCMalloc_SlowLock(LPLONG lockword) {
> +    Sleep(0);        // Yield immediately since fast path failed
> +    while (true) {
> +        if (!InterlockedExchange(lockword, 1))
> +            return;
> +
> +        Sleep(2);
> +    }
> +}

This could be a little simpler:

Sleep(0);
while (InterlockedExchange(lockword, 1))
    Sleep(2);
Comment 4 Patrick R. Gansterer 2010-07-23 08:36:40 PDT
Created attachment 62428 [details]
Patch

(In reply to comment #3)
> I don't understand how this code will be compiled for WinCE. The first branch of this #if/#elif chain is:
> 
> #if (CPU(X86) || CPU(X86_64) || CPU(PPC)) && (COMPILER(GCC) || COMPILER(MSVC))
> 
> Will WinCE not match that condition?
CPU(ARM)

> > +    LONG m_lockword;
> 
> I think m_lockword is supposed to be a volatile LONG.

No, otherwise you get "convert parameter 1 from 'volatile LONG *' to 'LPLONG'"
Comment 5 WebKit Review Bot 2010-07-23 08:38:25 PDT
Attachment 62428 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
JavaScriptCore/wtf/TCSpinLock.h:198:  "windows.h" already included at JavaScriptCore/wtf/TCSpinLock.h:55  [build/include] [4]
JavaScriptCore/wtf/TCSpinLock.h:200:  TCMalloc_SlowLock is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/TCSpinLock.h:205:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
JavaScriptCore/wtf/TCSpinLock.h:210:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
JavaScriptCore/wtf/TCSpinLock.h:214:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
JavaScriptCore/wtf/TCSpinLock.h:215:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
JavaScriptCore/wtf/TCSpinLock.h:225:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
JavaScriptCore/wtf/TCSpinLock.h:225:  TCMalloc_SlowLock is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/TCSpinLock.h:226:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 9 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Adam Roben (:aroben) 2010-07-23 09:05:47 PDT
(In reply to comment #4)
> Created an attachment (id=62428) [details]
> Patch
> 
> (In reply to comment #3)
> > I don't understand how this code will be compiled for WinCE. The first branch of this #if/#elif chain is:
> > 
> > #if (CPU(X86) || CPU(X86_64) || CPU(PPC)) && (COMPILER(GCC) || COMPILER(MSVC))
> > 
> > Will WinCE not match that condition?
> CPU(ARM)

Ah!

> > > +    LONG m_lockword;
> > 
> > I think m_lockword is supposed to be a volatile LONG.
> 
> No, otherwise you get "convert parameter 1 from 'volatile LONG *' to 'LPLONG'"

I guess InterlockedExchange has a different signature on WinCE.
Comment 7 WebKit Commit Bot 2010-07-23 09:24:52 PDT
Comment on attachment 62428 [details]
Patch

Clearing flags on attachment: 62428

Committed r63981: <http://trac.webkit.org/changeset/63981>
Comment 8 WebKit Commit Bot 2010-07-23 09:24:57 PDT
All reviewed patches have been landed.  Closing bug.