Re: Another patch to review

On Mon, Aug 5, 2013 at 9:55 PM, Ralph Campbell <[email protected]> wrote:
> Here is another change I'm planning to make.
> Feel free to comment.

As before: If you want to spend the time on it, I could see this as at
least three patches: (1) whitespace cleanup, (2) minor preparatory
fix-ups like 'unsigned int' to 'size_t', (3) folding QueueRunnable
into ThreadState.

More comments below.

> Since QueueRunnable and ThreadState have the same lifetimes,
> it seems simpler to me to eliminate QueueRunnable and make
> ThreadState inherit from Runnable.

ThreadState currently inherits from CS::Utility::AtomicRefCount,
whereas QueueRunnable inherits from plain csRefCount. Do we lose any
safety with this change?

Other than the above question, since ThreadState and QueueRunnable are
private implementation details of ThreadedJobQueue, and since it does
not seem to take any sort of advantage of them being separate, the
proposal seems reasonable.

> @@ -74,7 +72,10 @@
>      // Deallocate
>      for (size_t i = 0; i < numWorkerThreads; ++i)
>      {
> -      allThreadState[i]->runnable.Invalidate();
> +      // The Thread object holds a reference to this so there is a
> +      // reference count loop that we have to break.
> +      // It also means we don't have to IncRef() / DecRef() inside Run().

Does the comment about IncRef()/DecRef() have value here? That is,
will it enlighten someone who is not familiar with how the code worked
prior to this change? It certainly makes sense to mention it in the
commit message, but it's not clear if having it in-code will be
helpful to a newcomer reading the code.

> +      allThreadState[i]->threadObject.Invalidate();
>      }
>      delete[] allThreadState;
>    }

------------------------------------------------------------------------------
Get 100% visibility into Java/.NET code with AppDynamics Lite!
It's a free troubleshooting tool designed for production.
Get down to code-level detail for bottlenecks, with <2% overhead. 
Download for free and get started troubleshooting in minutes. 
http://pubads.g.doubleclick.net/gampad/clk?id=48897031&iu=/4140/ostg.clktrk