[Crystal-develop] Testing on Windows

[Ralph Campbell]

I have some changes for pthreads and some of those could be applied to
the win32_thread code but I don't have a Windows system to test on.
Any suggestions? Volunteers?

[Eric Sunshine]

On Sun, Aug 4, 2013 at 9:35 PM, Ralph Campbell <***@gmail.com> wrote:
> I have some changes for pthreads and some of those could be applied to
> the win32_thread code but I don't have a Windows system to test on.
> Any suggestions? Volunteers?

It's been several years since I last had a Windows installation. I
can't speak with authority for the other developers, but I believe
that most of them are running Linux. Frank (res) does maintain the
cs-win32libs package, however, that may be done via virtual machine.

If all else fails, and if you have Windows installation media, perhaps
you could test the win32_thread changes in a virtual machine?

[res]

> It's been several years since I last had a Windows installation. I
> can't speak with authority for the other developers, but I believe
> that most of them are running Linux. Frank (res) does maintain the
> cs-win32libs package, however, that may be done via virtual machine.

Yeah, I use a virtual machine.

> If all else fails, and if you have Windows installation media, perhaps
> you could test the win32_thread changes in a virtual machine?

Testing in a VM is very likely fine; threading semantics are generally defined by the OS and not the hardware.
(Of course, a VM installation may have scheduling/timing differences compared to a „real“ installation…
but these can just as well occur between different machines with „real“ installations.)
However, I also have a „real“ Windows installation, so I can test stuff for you.

-f.r.

[Ralph Campbell]

I started off running valgrind on some simple tests and the deeper I
dug, the more dirt I found. :-)
Basically, for each call to pthread_create(), there should be a matching
call to pthread_join() to recover system resources.
The current code for ThreadBase::Wait() checks IsRunning() to prevent
thread_join() from being called before pthread_create() is called.
Having the running/not-running state cleared by the child thread when
runnable->Run() returns in proxyFunc() means that if the child has
finnished, pthread_join() is never called.
Looking at the implementation more closely, I decided to simplify things
by removing the use of ThreadStartParams and Barrier, and making the
isRunning flag be set only by Start().
Or in other words, isRunning means threadHandle is valid.
I think this change also allows the thread to be Stop()'ed and Start()'ed
more than once although I haven't tested that.

I made similar changes to the win32 threads and I'm hoping
one of you will test it for me. :-)

If you can't apply it easily due to white space mangling, I can
resend as a tar or zip file.


Index: include/csutil/threading/pthread_thread.h
===================================================================
--- include/csutil/threading/pthread_thread.h (revision 39557)
+++ include/csutil/threading/pthread_thread.h (working copy)
@@ -41,11 +41,16 @@
public:
ThreadBase (Runnable* runnable);

+ ~ThreadBase ();
+
void Start ();

void Stop ();

- bool IsRunning () const;
+ bool IsRunning () const
+ {
+ return isRunning;
+ }

bool SetPriority (ThreadPriority prio);

@@ -55,6 +60,11 @@

static ThreadID GetThreadID ();

+ Runnable* GetRunnable () const
+ {
+ return runnable;
+ }
+
ThreadPriority GetPriority () const
{
return priority;
@@ -65,9 +75,8 @@

pthread_t threadHandle;

- int32 isRunning;
+ mutable bool isRunning;
ThreadPriority priority;
- Barrier startupBarrier;
};


Index: libs/csutil/threading/pthread_thread.cpp
===================================================================
--- libs/csutil/threading/pthread_thread.cpp (revision 39557)
+++ libs/csutil/threading/pthread_thread.cpp (working copy)
@@ -33,36 +33,12 @@

namespace
{
-
- class ThreadStartParams : public CS::Memory::CustomAllocated
+ static void* proxyFunc (void* param)
{
- public:
- ThreadStartParams (ThreadBase* thread, Runnable* runner, int32* isRunningPtr,
- Barrier* startupBarrier)
- : thread (thread), runnable (runner), isRunningPtr (isRunningPtr),
- startupBarrier (startupBarrier)
- {
- }
-
- ThreadBase* thread;
- Runnable* runnable;
- int32* isRunningPtr;
- Barrier* startupBarrier;
- };
-
- void* proxyFunc (void* param)
- {
// Extract the parameters
- ThreadStartParams* tp = static_cast<ThreadStartParams*> (param);
- csRef<ThreadBase> thread (tp->thread);
- int32* isRunningPtr = tp->isRunningPtr;
- Runnable* runnable = tp->runnable;
- Barrier* startupBarrier = tp->startupBarrier;
+ ThreadBase* tb = static_cast<ThreadBase*> (param);
+ Runnable* runnable = tb->GetRunnable();

- // Set as running and wait for main thread to catch up
- AtomicOperations::Set (isRunningPtr, 1);
- startupBarrier->Wait ();
-
#ifdef CS_HAVE_PTHREAD_SETNAME_NP
{
// Set the name, for debugging
@@ -71,13 +47,10 @@
pthread_setname_np (pthread_self(), threadName);
}
#endif
-
- // Run
+
+ // Run
runnable->Run ();

- // Set as non-running
- AtomicOperations::Set (isRunningPtr, 0);
-
return 0;
}

@@ -85,27 +58,33 @@


ThreadBase::ThreadBase (Runnable* runnable)
- : runnable (runnable), isRunning (0), priority (THREAD_PRIO_NORMAL),
- startupBarrier (2)
+ : runnable (runnable), isRunning (false), priority (THREAD_PRIO_NORMAL)
{
}

+ ThreadBase::~ThreadBase ()
+ {
+ if (IsRunning ())
+ {
+ pthread_join (threadHandle, 0);
+ }
+ }
+
void ThreadBase::Start ()
{
if (!IsRunning ())
- {
- ThreadStartParams param (this, runnable, &isRunning, &startupBarrier);
+ {
+ int res;

- pthread_attr_t attr;
- pthread_attr_init(&attr);
- pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_JOINABLE);
- pthread_create(&threadHandle, &attr, proxyFunc, &param);
- pthread_attr_destroy(&attr);
-
- startupBarrier.Wait ();
+ // The default is a joinable thread.
+ res = pthread_create(&threadHandle, 0, proxyFunc, this);
+ if (res == 0)
+ {
+ isRunning = true;

- // Set priority to make sure its updated if we set it before starting
- SetPriority (priority);
+ // Set priority to make sure its updated if we set it before starting
+ SetPriority (priority);
+ }
}
}

@@ -113,25 +92,18 @@
{
if (IsRunning ())
{
- int res = pthread_cancel (threadHandle);
- if (res == 0)
- {
- AtomicOperations::Set (&isRunning, 0);
- }
+ isRunning = false;
+ pthread_cancel (threadHandle);
+ pthread_join (threadHandle, 0);
}
}

- bool ThreadBase::IsRunning () const
+ bool ThreadBase::SetPriority (ThreadPriority prio)
{
- return (AtomicOperations::Read ((int32*)&isRunning) != 0);
- }
+ int res = 0;

- bool ThreadBase::SetPriority (ThreadPriority prio)
- {
- int res = 1;
-
if (IsRunning ())
- {
+ {
struct sched_param SchedulerProperties;

// Clear the properties initially
@@ -156,23 +128,24 @@
}
}

- if (res != 0)
+ if (res == 0)
{
priority = prio;
}

- return res != 0;
+ return res == 0;
}

void ThreadBase::Wait () const
{
if (IsRunning ())
{
- pthread_join (threadHandle,0);
+ isRunning = false;
+ pthread_join (threadHandle, 0);
}
}

- void ThreadBase::Yield ()
+ void ThreadBase::Yield ()
{
sched_yield ();
}
Index: include/csutil/threading/win32_thread.h
===================================================================
--- include/csutil/threading/win32_thread.h (revision 39570)
+++ include/csutil/threading/win32_thread.h (working copy)
@@ -64,6 +64,11 @@

static CS::Threading::ThreadID GetThreadID ();

+ Runnable* GetRunnable () const
+ {
+ return runnable;
+ }
+
ThreadPriority GetPriority () const
{
return priority;
@@ -82,9 +87,7 @@
mutable void* threadHandle;
uint threadId;

- int32 isRunning;
ThreadPriority priority;
- Barrier startupBarrier;

static unsigned int __stdcall proxyFunc (void* param);
};
Index: libs/csutil/threading/win32_thread.cpp
===================================================================
--- libs/csutil/threading/win32_thread.cpp (revision 39565)
+++ libs/csutil/threading/win32_thread.cpp (working copy)
@@ -107,40 +107,12 @@
namespace Implementation
{

- namespace
+ static unsigned int __stdcall ThreadBase::proxyFunc (void* param)
{
-
- class ThreadStartParams : public CS::Memory::CustomAllocated
- {
- public:
- ThreadStartParams (ThreadBase* thread, Runnable* runner, int32* isRunningPtr,
- Barrier* startupBarrier)
- : thread (thread), runnable (runner), isRunningPtr (isRunningPtr),
- startupBarrier (startupBarrier)
- {
- }
-
- ThreadBase* thread;
- Runnable* runnable;
- int32* isRunningPtr;
- Barrier* startupBarrier;
- };
-
- }
-
- unsigned int __stdcall ThreadBase::proxyFunc (void* param)
- {
// Extract the parameters
- ThreadStartParams* tp = static_cast<ThreadStartParams*> (param);
- csRef<ThreadBase> thread (tp->thread);
- int32* isRunningPtr = tp->isRunningPtr;
- Runnable* runnable = tp->runnable;
- Barrier* startupBarrier = tp->startupBarrier;
+ ThreadBase* tb = static_cast<ThreadBase*> (param);
+ Runnable* runnable = tb->GetRunnable ();

- // Set as running and wait for main thread to catch up
- AtomicOperations::Set (isRunningPtr, 1);
- startupBarrier->Wait ();
-
// Set the name, for debugging
SetThreadName ((DWORD)-1, runnable->GetName ());

@@ -150,15 +122,12 @@
// Clean up TLSed objects
CleanupAllTlsInstances ();

- // Set as non-running
- AtomicOperations::Set (isRunningPtr, 0);
-
return 0;
}

ThreadBase::ThreadBase (Runnable* runnable)
- : runnable (runnable), threadHandle (0), threadId (0), isRunning (0),
- priority (THREAD_PRIO_NORMAL), startupBarrier (2)
+ : runnable (runnable), threadHandle (0), threadId (0),
+ priority (THREAD_PRIO_NORMAL)
{
}

@@ -171,16 +140,12 @@
{
if (!threadHandle)
{
- ThreadStartParams param (this, runnable, &isRunning, &startupBarrier);
-
// _beginthreadex does not always return a void*,
// on some versions of MSVC it gives uintptr_t
// and therefor needs a reinterpret_cast.
threadHandle = reinterpret_cast<void*> (_beginthreadex (0, 0, &proxyFunc,
- &param, 0, &threadId));
+ this, 0, &threadId));

- startupBarrier.Wait ();
-
// Set priority to make sure its updated if we set it before starting
SetPriority (priority);
}
@@ -194,14 +159,13 @@
if (res == 0)
{
threadHandle = 0;
- AtomicOperations::Set (&isRunning, 0);
}
}
}

bool ThreadBase::IsRunning () const
{
- return (AtomicOperations::Read ((int32*)&isRunning) != 0);
+ return threadHandle != 0;
}

bool ThreadBase::SetPriority (ThreadPriority prio)

[Eric Sunshine]

CS development is somewhat quiescent these days, so please do not feel
snubbed if people don't respond to code review requests immediately or
at all. If you feel confident about your changes, sometimes the most
pragmatic approach is to commit them and then monitor crystal-main,
crystal-develop, and crystal-tracker mailing lists for problem
reports. (Bug reports on Trac get posted to cryatal-tracker.)

Regarding this patch, it might be easier to review if split into
several pieces. For instance, there are a number of whitespace-only
changes which make the patch noisy, plus some other changes not
necessarily dependent upon each other, which could be split apart. For
instance, I could envision separate patches: (1) general whitespace
cleanup, (2) drop unnecessary pthread_attr in favor of default values,
(3) refactoring/simplification of pthread implementation, (4) ditto
for win32 implementation. If you're using git for development, then
it's pretty easy to organize your patches like this. If not, then
splitting the patch might be more hassle than it's worth.

More comments below...

On Mon, Aug 5, 2013 at 4:19 PM, Ralph Campbell <***@gmail.com> wrote:
> I started off running valgrind on some simple tests and the deeper I
> dug, the more dirt I found. :-)
> Basically, for each call to pthread_create(), there should be a matching
> call to pthread_join() to recover system resources.
> The current code for ThreadBase::Wait() checks IsRunning() to prevent
> thread_join() from being called before pthread_create() is called.
> Having the running/not-running state cleared by the child thread when
> runnable->Run() returns in proxyFunc() means that if the child has
> finnished, pthread_join() is never called.

Diagnosis makes sense.

> Looking at the implementation more closely, I decided to simplify things
> by removing the use of ThreadStartParams and Barrier, and making the
> isRunning flag be set only by Start().
> Or in other words, isRunning means threadHandle is valid.

This changes semantics a bit since we no longer report whether the
underlying pthread is still running. Do any of the existing clients in
CS care about this slight difference in behavior? Can we make a guess
about external clients?

Without making an exhaustive audit, my guess would be that the
semantic change should not matter to clients and that this change
should be safe.

> I think this change also allows the thread to be Stop()'ed and Start()'ed
> more than once although I haven't tested that.
>
> I made similar changes to the win32 threads and I'm hoping
> one of you will test it for me. :-)

If nobody steps forward, and if you can't test it yourself via virtual
machine, the above advice might be reasonable: commit and wait for
someone to complain. (It's not great advice, but...)

> If you can't apply it easily due to white space mangling, I can
> resend as a tar or zip file.

Overall, the approach taken by the patch seems sound enough, nor I did
spot any obvious flaws in my (perhaps less than thorough) review.

-- ES

[Ralph Campbell]

Thanks for the comments.
I agree with them and I'm used to doing Linux development via patches so
the splitting & white space comments make sense.
I wasn't expecting speedy responses. I probably would have waited a week
and then done what you suggest, commit and track comments.

The run state is only being used internally to track whether Wait()
and such need to clean up state. Since the clean up needs to happen
whether or not the thread has exited, the IsRunning() call would
now just indicate whether it was started or not.
The actual use is that all the callers call Wait() to be sure
a thread is finished.

On Tue, 2013-08-06 at 11:56 -0400, Eric Sunshine wrote:
> CS development is somewhat quiescent these days, so please do not feel
> snubbed if people don't respond to code review requests immediately or
> at all. If you feel confident about your changes, sometimes the most
> pragmatic approach is to commit them and then monitor crystal-main,
> crystal-develop, and crystal-tracker mailing lists for problem
> reports. (Bug reports on Trac get posted to cryatal-tracker.)
>
> Regarding this patch, it might be easier to review if split into
> several pieces. For instance, there are a number of whitespace-only
> changes which make the patch noisy, plus some other changes not
> necessarily dependent upon each other, which could be split apart. For
> instance, I could envision separate patches: (1) general whitespace
> cleanup, (2) drop unnecessary pthread_attr in favor of default values,
> (3) refactoring/simplification of pthread implementation, (4) ditto
> for win32 implementation. If you're using git for development, then
> it's pretty easy to organize your patches like this. If not, then
> splitting the patch might be more hassle than it's worth.
>
> More comments below...
>
> On Mon, Aug 5, 2013 at 4:19 PM, Ralph Campbell <***@gmail.com> wrote:
> > I started off running valgrind on some simple tests and the deeper I
> > dug, the more dirt I found. :-)
> > Basically, for each call to pthread_create(), there should be a matching
> > call to pthread_join() to recover system resources.
> > The current code for ThreadBase::Wait() checks IsRunning() to prevent
> > thread_join() from being called before pthread_create() is called.
> > Having the running/not-running state cleared by the child thread when
> > runnable->Run() returns in proxyFunc() means that if the child has
> > finnished, pthread_join() is never called.
>
> Diagnosis makes sense.
>
> > Looking at the implementation more closely, I decided to simplify things
> > by removing the use of ThreadStartParams and Barrier, and making the
> > isRunning flag be set only by Start().
> > Or in other words, isRunning means threadHandle is valid.
>
> This changes semantics a bit since we no longer report whether the
> underlying pthread is still running. Do any of the existing clients in
> CS care about this slight difference in behavior? Can we make a guess
> about external clients?
>
> Without making an exhaustive audit, my guess would be that the
> semantic change should not matter to clients and that this change
> should be safe.
>
> > I think this change also allows the thread to be Stop()'ed and Start()'ed
> > more than once although I haven't tested that.
> >
> > I made similar changes to the win32 threads and I'm hoping
> > one of you will test it for me. :-)
>
> If nobody steps forward, and if you can't test it yourself via virtual
> machine, the above advice might be reasonable: commit and wait for
> someone to complain. (It's not great advice, but...)
>
> > If you can't apply it easily due to white space mangling, I can
> > resend as a tar or zip file.
>
> Overall, the approach taken by the patch seems sound enough, nor I did
> spot any obvious flaws in my (perhaps less than thorough) review.
>
> -- ES
>
> ------------------------------------------------------------------------------
> Get your SQL database under version control now!
> Version control is standard for application code, but databases havent
> caught up. So what steps can you take to put your SQL databases under
> version control? Why should you start doing it? Read more to find out.
> http://pubads.g.doubleclick.net/gampad/clk?id=48897031&iu=/4140/ostg.clktrk
> _______________________________________________
> Crystal-develop mailing list
> Crystal-***@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/crystal-develop

[Eric Sunshine]

On Tue, Aug 6, 2013 at 9:04 PM, Ralph Campbell <***@gmail.com> wrote:
> Thanks for the comments.
> I agree with them and I'm used to doing Linux development via patches so
> the splitting & white space comments make sense.

It's not a requirement of this project, but if it's something you're
used to, I personally would be pleased to see commits made in the
style of the Linux kernel or similar projects: one well-defined,
small, easily digestible topic per commit; first line of commit
message a short summary of change; a well written commit message which
explains the problem and justifies the solution. (At least a couple of
us use git for CS work, so commit messages formatted in the style
suggested by git make for nicer display in the git tool ecosystem.)

> I wasn't expecting speedy responses. I probably would have waited a week
> and then done what you suggest, commit and track comments.

Sounds reasonable. Unfortunately, these days, the project is rather
short of developers, so it can be difficult to get reviews or input.
Developers who are around have indicated (off-list) that they are
comfortable with the quality of your patches, so, if you are confident
about your changes, you should feel free to commit them.

> The run state is only being used internally to track whether Wait()
> and such need to clean up state. Since the clean up needs to happen
> whether or not the thread has exited, the IsRunning() call would
> now just indicate whether it was started or not.
> The actual use is that all the callers call Wait() to be sure
> a thread is finished.

Indeed, and a client's use of IsRunning() for anything other would be
subject to race conditions anyhow -- with or without this patch -- so
the new slightly different semantic wouldn't harm any such client (if
there was one) beyond the harm it's already doing to itself by relying
upon IsRunning().

-- ES