Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

I am engaging in redesigning a linux c++ server application. The application acts as file relayer in the form of receiving file packets (incuding control packets and data packets) from client A, writing these packets into a local data file then create and update index file, finally start many threads to read the file data and forward these data packets to its receivers such as client B, client C, client D, etc. Flow chart is like: Client A -> Server -> Client B, Client C, Client D, ...

Currently the application is using Producer-Consumer pattern to pass data packets among modules, here are some modules we have: SocketModule(contains a thread for listening and reading incoming data of all sockets), SessionModule(for managing user tcp sessions), FileRelayModule(contains many threads for doing jobs like processing incoming packets and writing into local data file, reading local data files and updating index files and forwarding these data packets to receivers via TCP sockets).

The problem is that its performance is so bad, take an example, it takes about 15 minutes for Client A to send a file of size 200M to Client B, but I did a p2p file sending and receiving between different machines it takes only 20 seconds.

We think the main cause of bad performance is that we store sockets information in a global std::map, then every thread who wants to send out data (to clients) needs to wait for the lock for std::map, thus the application acts like single-threaded application.

As the original design of this application doesn't follow principles of design pattern and this results in many issues, so we want redesign it using MVC pattern, adding roles of thread manager and file manager. The first item I think of is to use a ThreadPool, then dispatch every accepted socket into the thread who has the least clients(sockets number). Is this possible or reasonable for this application? What would you do if you were the designer of this project?

[update]
1. I will check the project and figure out which part is the real bottleneck of performance
2. No matter which part is the real bottleneck, we need to reconstruct the application for it to manage the resources such as threads and memory and be easy to maintain.

I am gonna do things like following:
1. Create many classes: CSessionManager for managing tcp session data (contains a thread), CFileManager for doing file relaying and managing all data files and index files (contains a ThreadPool with 32 threads of which contains a sockets map, and a listening socket), CFileServer(contains the CSessionManager and CFileManager). Here CFileServer acts as Controller, the CFileManager acts as Model.
2. The CFileManager will create a thread pool with 32 threads initialized at the startup. The CFileServer creates a listening socket and puts every accepted socket to CFileManager where the the new accepted socket is set to a thread in the threadpool by querying who has the least sockets.

[update2]
The threadpool will be designed as following(is it reasonable?):

               std::list<CThread*> workers;
               //each thread would be
               class CThread{
                   public: 
                     PostMsg(CMessage*);
                     static void Run();

               private:
                     map<int, socketinfo*> sockets;
                     std::deque<CMessage*> messageQueue;
               };

               ...
               Other objects will call PostMsg to inform message
               to this CThread and in the run function:
               void CThread::Run(){
                     ....
                     while(1){
                         ....
                         //firstly process all messages
                         //these messages includes adding socket,
                         //deleting socket, or go offline
                         CMessage* msg = messageQueue.pop_front;
                         if (msg != NULL) {
                            process(msg);
                         }

                         //after processing the messages,
                         //now read/write the sockets data
                         //set readfd and writefd
                         ret = select(max, readfd, writefd, NULL, timeout);

                         //now process readfd and writefd
                         //read data from readfd, then write to local 
                         //file read data from local file then write 
                         //to wirtefd or post a message to controller
                        // to remove fd from array.
                         ......
                     }
               }
share|improve this question
1  
Well, I would not expect anyone here to give you a meaningful answer. This kind of question can only be answered by analyzing your existing application's structure in-depth. However, do you only guess where the bottleneck is? Or did you made some real profiling? That's what you really should do before you invest any time in optimizing the wrong part of the application. –  Doc Brown Jun 15 '13 at 6:31
1  
You think you know the reason for the bad performance? Find out for sure before you do anything, or you are likely wasting your time. Programmers are notoriously bad at guessing performance bottlenecks. –  Philipp Jun 15 '13 at 8:11
    
@StevePeng Regarding your update: the idea behind a ThreadPool is not to have them maintain state and keep their private queue of jobs. That is just a collection of threads. The real idea is that you don't know which jobs take more time: a thread with 100 sockets can be faster than one with 10. So you "start" a thread for each socket. To avoid thread starvation and the overhead of thread creation, you don't actually create them, but have a bunch of them ready in a Pool. Forcing your own scheduling on the threads only complicates things. –  Jer Jun 16 '13 at 7:24
    
@StevePeng More regarding your update: in MVC, the sockets would be the Views. The Model shouldn't create them. The only thing the Model should be aware of is that there are Observers. So you create the CFileManager as Model. You attach the CFileServer as Controller/Observer. The Controller creates Views and tells the Model that it has new Observers. Then the Views query the Model for initial data. Or the Controller tells the Model to prepare some data for these Views. Whenever the Model has updates or data ready, it notifies its Observers. –  Jer Jun 16 '13 at 7:44
    
@StevePeng Regarding your update2, that is not a ThreadPool, so if you intended that, then it's not really reasonable, no. The ThreadPool pattern lets you separate different responsibilities and concerns. The ThreadPool manages threads and their lifetimes. The TaskQueue holds tasks. A Thread takes a task from the TaskQueue and handles it. Other parts of the program don't talk to threads. They put stuff in the TaskQueue. Ideally, the ThreadPool is generic, so that you give it a TaskHandler object, and you can use it multiple times in your program. –  Jer Jun 19 '13 at 18:15
add comment

1 Answer

up vote 4 down vote accepted

If I were the designer of this project, I would focus on the performance first, and then any design patterns.

Performance improvement starts with knowing where the bottlenecks are, not where you think they are. You could spend many hours (or days, weeks) optimizing a part of the program that does not influence the overall performance at all. There are tools for profiling concurrent programs, but I usually just let the threads write some log info about where they spend their time.

First of all, the benchmark time. The P2P situation with which you compare is not exactly equal to your own application. In the P2P case, the sending client already had the file, whereas in your application's case, that file needs to be uploaded first. From what I gather, this upload stage is a single-threaded process too, and it is worthwhile to check how long it takes. A comparable situation would be using FTP to upload the file, and then P2P to distribute it. Also, timings over a local network would be vastly different from timings over the internet.

With that out of the way, though, I'd say that a factor 15 improvement is a nice goal.

At first glance, I see the following potential bottlenecks:

  1. The file is read in its entirety, and then further processed, before distribution starts. Perhaps this process can also be made concurrent, and perhaps sending file blocks to clients can start as soon as you have them, adding the extra information later. If need be, the client can do its own reconstruction of the complete file (just as with P2P).

  2. Disk I/O. It looks like the file is written to disk, and each thread is performing disk I/O to read it. This may be sped up with caching or memory mapping. You may also want to check that your disk hardware can handle the I/O.

  3. The std::map is a horrible data structure quite on its own, let alone when it is locked. It is a balanced red-black tree, so each insert or remove may trigger a rebalance, and each search is O(lg n). On top of that, memory allocation may seriously impact performance. What you would like is to see each socket as a Task, and have unfinished tasks in a simple TaskQueue. A (double-)linked list (possibly with a memory pool) or a std::vector acting as stack is enough. That will reduce the wait time to O(1) with a very low constant.

  4. Socket I/O. Your threads may be waiting a long time before the I/O call with the clients is finished. By setting it up as asynchronous calls, they can handle writing to a socket, put the socket on the TaskQueue, and let another thread handle the callback.

  5. A ThreadPool is a good idea, but I wouldn't burden each thread with multiple clients. Let them read a simple task from the TaskQueue, perform it quickly, and then insert the updated task back into the TaskQueue. Each thread just does a different thing based on some "command" in the task.

  6. Last but not least, is the file binary or compressed? Reducing the size of data to transfer is also a valid optimization.

I think that what you are looking for in MVC is the responsiveness and concurrency, the "feel" that it acts fast, as found in a lot of MVC applications. That is however due, in part, to the messaging system that MVC uses, and its implementation. (The other parts are that different Views and Controllers can behave concurrently, and that the Model is non-blocking.) MVC in itself guarantees no performance gain.

So I would focus on the underlying patterns, that may even be used by MVC itself: ThreadPools, TaskQueue, and simultaneously asynchronous I/O and caching.

With that in place, I don't see a real use for MVC. MVC is an abstraction for different Views and different Controllers, that show and act upon the Model in different ways. What you have, however, is a lot of the same. So instead of, say, the Model telling each thread or client that it has a file ready, it can just put so many tasks on the queue, and not care about which process handles what exactly. As long as the queue is empty at the end of the process, you're okay.

Anyway, as I said, before you do anything, make sure that you know what the actual bottlenecks are.

share|improve this answer
    
added one more section -- [update2] –  Steve Jun 18 '13 at 23:40
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.