Revamping Code Completion with little changes

[zieQ]

Why a singleton? You could have many different pools (if needed). Why restrict yourself to just one global pool?
One pool for code-completion, one for compiler, etc.

And the compiler could use thread pooling. Each wxExecute could be executed in a thread pool worker class...
Yiannis.

Well, singleton due to the pool dimensioning. There is an optimal value for the number of threads for the best performance that would not be guaranteed if there are many pools, especially if the dimension is allowed to vary during runtime by analyzing the workload. Did you read the second link I posted? I may be wrong however, we may propose a simplified version with no adjustement of the number of threads but I don't think that many pools is a good idea anyway. To me, thread pools are a collection of threads (optimal number dependending on the machine and the type of workload, which vary during runtime in our case) and a collection of (independent) tasks to be accomplished by the given number of threads. If tasks are independent, there's no need to separate pools for each kind of task to perform. We then can mix parsing with compilation, etc.

Second remark, I don't see any way for the compiler to use the thread pool. AFAIK, there is no method that link a wxThread to a wxProcess and conversely, and wxExecute deals with wxProcess while the thread pools deals with wxThread. Can you explain how you would do it? Maybe I am blind :p

OK, but if the code completion is going to use the thread pool, how long will I have to wait before the pool's ready so I can make the changes to CC? Or should I begin the changes and later we can switch to using the pool?

I think we can define the public interface fast for the pool so you can start redesigning it before the pool is implemented... Just my thought...

[rickg22]

AHHHHHHH so something like...

Code

class ParserTask: public ThreadTask
{
};

?

[mandrav]

OK, here's a *primitive* I 'm working on. It's nowhere near complete API-wise. But hey, let's add some flesh and bones in the discussion
(don't mind the STL in there - we 'll probably use wx replacements for those)

Code

#ifndef CBTHREADPOOL_H
#define CBTHREADPOOL_H

#include <queue>
#include <vector>
#include <wx/event.h>
#include <wx/thread.h>

extern int idThreadPoolMsg;

// flags
#define DELETE_WORKER_WHEN_DONE 0x01

/// Abstract base class for workers assigned to the thread pool object
/// Derive a class from this and perform your thing in Execute()
/// It's like wxThread, except it's not a thread anymore ;)
class cbThreadPoolWorker
{
	public:
		virtual int Execute() = 0;
};

// forward decl
class PrivateThread;

/// Definition of  a Thread Pool Object (used by thread pool only)
struct cbThreadPoolObject
{
    cbThreadPoolObject() : pThread(0), pWorker(0), flags(0) {}
    cbThreadPoolObject(const cbThreadPoolObject& rhs) : pThread(rhs.pThread), pWorker(rhs.pWorker), flags(rhs.flags) {}
    PrivateThread* pThread;
    cbThreadPoolWorker* pWorker;
    int flags;
};

/// The main thread pool object.
class cbThreadPool
{
    public:
        cbThreadPool(wxEvtHandler* owner, int id = -1, int concurrentThreads = -1);
        virtual ~cbThreadPool();

        virtual void SetConcurrentThreads(int concurrentThreads);
        virtual bool AddThread(cbThreadPoolWorker* worker, bool deleteWhenDone = false);
        virtual void RemoveThread(cbThreadPoolWorker* worker);
    protected:
        virtual void AllocThreads();
        virtual void FreeThreads();
        virtual void ScheduleThreads();
        void Log(const wxString& msg);

        wxEvtHandler* m_pOwner;
        int m_ID;
        std::queue<cbThreadPoolObject> m_Pool;
        std::vector<PrivateThread*> m_Workers; // worker threads
        int m_ConcurrentThreads;
        int m_MaxThreads;
};

/// Base thread class
class PrivateThread : public wxThread
{
	public:
        enum State
        {
            Idle,
            Busy
        };
        enum Status
        {
            LaunchJob,
            Quit
        };
		PrivateThread(cbThreadPoolWorker* worker = 0)
            : m_Condition(m_Mutex),
            m_pWorker(worker),
            m_Status(Quit),
            m_State(Idle)
        {
            m_Mutex.Lock();
        }
		~PrivateThread(){}

		void SetWorker(cbThreadPoolWorker* worker);
		void SetStatus(Status status);
		State GetState(){ return m_State; }
		virtual ExitCode Entry();
	protected:
        friend class cbThreadPool; // allow thread pool to access the mutex and the condition object
		wxMutex m_Mutex;
		wxCondition m_Condition;
		cbThreadPoolWorker* m_pWorker;
		Status m_Status;
		State m_State;
	private:
};

#endif // CBTHREADPOOL_H

The way it would work is like:

Code

class ParserThread : public cbThreadPoolWorker
{
    ...
    int Execute(); // override pure virtual
    ...
};

// set the max nr of concurrent threads to run
// -1 means "number of CPUs" (yes it works with HyperThreading ;) )
g_ThreadPool.SetConcurrentThreads(-1);

// pass a new "thread" to the pool; it 'll run when the pool has available resources
g_ThreadPool.AddThread(new ParserThread);

Yiannis.

[Urxae]

Code

// flags
#define DELETE_WORKER_WHEN_DONE 0x01

/// Abstract base class for workers assigned to the thread pool object
/// Derive a class from this and perform your thing in Execute()
/// It's like wxThread, except it's not a thread anymore ;)
class cbThreadPoolWorker
{
	public:
		virtual int Execute() = 0;
};

If you're going to call delete on pointers to workers (as that #define implies) you should give them a virtual destructor, so that the derived class gets to run its own constructor as well .

[mandrav]

If you're going to call delete on pointers to workers (as that #define implies) you should give them a virtual destructor, so that the derived class gets to run its own constructor as well .

Right. Although it's early, I can't believe I 've missed it (you 'll see it in cbThreadPool).
Thanks

Yiannis.

[Urxae]

Code

#define DELETE_WORKER_WHEN_DONE 0x01
[...]
        virtual bool AddThread(cbThreadPoolWorker* worker, bool deleteWhenDone = false);

I just now saw that second line. What is the purpose of the #define if there is a parameter that seems to specify the exact same thing?

And you might want to rename AddThread and RemoveThread to something like AddJob and RemoveJob. From what I understand of thread pools you're not adding threads but jobs (or perhaps workers, but jobs sounds better ) which are then perfomred/executed by the pooled threads.

[zieQ]

I would rename cbThreadPoolWorker to cbThreadedTask (or cbTask) since thread pool worker may be misunderstood as the thread that performs the task. For me, a thread live for long, whereas a task is short, but it is cosmetics. Moreover, I would hide the implementation details (PrivateThread, cbThreadPoolObject) in the cpp file and just keep forward declarations if necessary. And I don't like the #define CONSTANT...

[zieQ]

Humm, a job is a list of tasks :p

[mandrav]

You have to realize this is just a draft which I 'm working on. It's nowhere near completion and many things will change and/or move around.
I didn't post it so that Rick or anyone else could start working on revamping cc, but just for discussion...

Code

#define DELETE_WORKER_WHEN_DONE 0x01
[...]
 virtual bool AddThread(cbThreadPoolWorker* worker, bool deleteWhenDone = false);

I just now saw that second line. What is the purpose of the #define if there is a parameter that seems to specify the exact same thing?

See? That's what I 'm talking about. Just to avoid the confusion, the AddThread()'s "delete" parameter tells cbThreadPool that this "thread" should be deleted on completion. The DELETE_WORKER_WHEN_DONE is set in cbThreadPoolObject.flags for this "thread" so that when it is done, it knows to delete it. In other words, these are two different steps of the process...
Instead of having " bool cbThreadPoolObject::deleteWhenDone;", we have a "int cbThreadPoolObject::flags;" as a bitmask...

Yiannis.

[rickg22]

I'd suggest using ManagedThread instead of wxThread... after all i created those for a reason
Unless there are objections of course...

[grv575]

I don't think it should be a singleton.  The pool of threads executes a queue of tasks, so independent modules (compile, code completion) would not want to wait to even start to execute if the other module has all the available threads occupied...

So multiple thread pools looks needed.  Is it possible to do something like:
#define THREADPOOL_COUNT 2

cpus = g_ThreadPool.GetCpuCount();
g_ThreadPool.SetConcurrentThreads(cpus / THREADPOOL_COUNT);

[Urxae]

So multiple thread pools looks needed.  Is it possible to do something like:
#define THREADPOOL_COUNT 2

cpus = g_ThreadPool.GetCpuCount();
g_ThreadPool.SetConcurrentThreads(cpus / THREADPOOL_COUNT);

You do realize that would round to 0 if there's only one CPU, right? :?

[zieQ]

I don't think it should be a singleton.  The pool of threads executes a queue of tasks, so independent modules (compile, code completion) would not want to wait to even start to execute if the other module has all the available threads occupied...

Well, could I suggest you to read to links I provided? If there are too many threads, even if you dispose of another pool, your tasks will wait for the processors to be available since they are already busy with the other pool btw, and you will also have a performance penalty. One thread pool make it easier to get optimal performance whatever the task could be (compile, code completion). Remember, the thread pool has a queue of tasks: if you don't flood the pool with tasks, independent modules could execute concurrently with maximum performance. We may assign a priority to tasks but that's not that easy to make it work well. Your solution is worse: you dispose of half the computing power for each pool, with half worker threads going idle most of the time due to inactivity of the pool! Nonsense.

Mandrav, I don't see why we may delete tasks since they are supposed to be executed for a short time? It's not safe I think, since once the task is completed, the pointer could have been deleted!

[grv575]

Your solution is worse: you dispose of half the computing power for each pool, with half worker threads going idle most of the time due to inactivity of the pool! Nonsense.

Yeah, I wasn't thinking.  I guess only 1 pool makes sense.  It's not like there's any other tasks besides compilation that should require long completion times anyway.

Mandrav, I don't see why we may delete tasks since they are supposed to be executed for a short time? It's not safe I think, since once the task is completed, the pointer could have been deleted!

You're going to want some way to signal the thread to terminate safely to cleanup resources (like say when the user closes the app).

[rickg22]

You're going to want some way to signal the thread to terminate safely to cleanup resources (like say when the user closes the app).

ManagedThread does that . It keeps a static flag, and each class can point to an additional flag that tells them to terminate.