patchCERTI - Patches: patch #7704, replace local full-weight...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

patch #7704: replace local full-weight processes with threads

Submitter:  Eugene Chemeritskiy <tyz>
Submitted:  Fri 20 Jan 2012 05:56:00 AM UTC
   
 
Category:  CERTI Priority:  5 - Normal
Status:  Postponed Privacy:  Public
Assigned to:  None Open/Closed:  Open

Jump to the original submission

Sat 25 Aug 2012 01:15:26 PM UTC, comment #9: 

Eric,

Thanks for your fast reaction.

As about global write access, I will keep this in mind and double check my current branch before commits.

Eugene Chemeritskiy <tyz>
Group Member
Sat 25 Aug 2012 12:57:28 PM UTC, comment #8: 

Hi Eugene,

I just created  the branch.
I added you as a developer as well that way you can commit to
the branch.

Please be gentle with the commit access, Savannah does not
makes it possible to grant access to only 'part' of a repo.

So you have commit right "globally", I just ask you
to only commit in the branch.

Are you OK with that?

Eric NOULARD <erk>
Group administrator
Sat 25 Aug 2012 11:43:48 AM UTC, comment #7: 

Eric,

It seems that I somehow misunderstood you. Of course I do not have permission to make a branch in CERTI CVS rep. Could you, please, make br_DEV_CERTI_threaded branch for me?

Eugene Chemeritskiy <tyz>
Group Member
Fri 24 Aug 2012 07:16:04 AM UTC, comment #6: 

Hi Eric,

I've created independent libRTIA subdirectory, because in addition to both RTIA and libRTI multi-threaded LRC includes some extra sources. Thus, it results into some kind of new CERTI component and deserves the similar separation. However, I do not have any objections of moving it anywhere. As about the future prospective, I think it will be a nice catch to include multi-threaded LRC not only as a build, but as a run time option either. The internal RTIA-RTIG protocol is remains the same, so one execution can include both multi-process and multi-threaded LRCs. Thus, it seems quite reasonable to separate RTIA core into an independent library for convenient use in both cases.

>> "The polling I used is also an attempt to set aside the problem I know how to solve in favor of the those I do not"

Here I tried to express that I knew how to implement the considerably clean and correct solution in case of related waiting as well as I knew nothing about internal CERTI component communications. So I've made a kind of kludge to get the proof-of-concept multi-threaded LRC.

I understand your reluctance of including dependency on third-party library in CERTI. However, it seems quite strange to reject boost usage. Currently multi-threaded LRC version is considered as a build option, so the legacy configuration will not require boost at all. Although it is not too difficult to implement threading for one platform only, it will discriminate users of other platforms. If the user really wants to get multi-threaded LRC, he can easily get boost library for an appropriate platform. But there is no hope to do it in case of platform dependent code. Another option is to implement your own compile-time condition-based code for multiple platforms. Currently there are several lines of code in this patch, but the idea does not seem to be a fast fix already. Moreover, if you will try to use threads inside of RTIG in future, you likely require a mature full-featured threading library, and its development will be a crucial overhead for a small researcher group. To be honest, I do not think that it is a good idea to reject boost thread (or any of its competitors) even for now.

The development plan you suggested is quite reasonable. Thanks for your review and constructive criticism. I'm going to make the new branch as soon as I get enough time for it.

Eugene Chemeritskiy <tyz>
Group Member
Tue 21 Aug 2012 12:59:29 PM UTC, comment #5: 

Hi Eugene,

...Patch review continued...

Concerning libRTIA or no libRTIA, if the code in there
is to be shared between RTIA and libRTI (or RTIG) then
it should certainly go to libCERTI, no need to create
a new libRTI.

If ever the code is independent enough, like
a "simple-thread-safe" ring,buffer then it could
even end-up in libHLA.

LibHLA may contains any RTI-agnostic code which could be useful
to Federate as well. e.g. Generic  "MessageBuffer" is in there.

by the way the naming "MsgBuf" for your class is unfortunately
too close to our current "MessageBuffer".

I suggest to name it "TSInMemoryMsgQueue" or something similar.
It could even be TSInMemoryQueue<typename T> with

TSInMemoryMsgQueue being TSInMemoryQueue<certi::MessageBuffer*>.

You said:
"The polling I used is also an attempt to set aside the problem I know how to solve in favor of the those I do not"

I don't get it what do you mean?

I think you get all my remark now.
Currently I cannot accept the usage of boost in CVS HEAD.
Now I agree with the "we should made a working patch first".

So I can propose you to create a br_DEV_CERTI_threaded
branch in which I can merge your work, so that it can
be easily accessible to you, me and others.
Then we work on this branch together in order to:

   1) Try to avoid boost usage
      This should not be a big deal if we:
            a) Target Linux/POSIX thread only in the first place
            b) Use in-memory ring buffer

   2) Merge part of it to HEAD when we are ok.

   3) In the end go back to HEAD with a unified approach.

Would you be OK with this process?

Eric NOULARD <erk>
Group administrator
Mon 20 Aug 2012 06:41:24 AM UTC, comment #4: 

Hi Eric,

Thanks for your comments.

Built-in CERTI tests require some extra packages and I had troubles while building it on PC I did not have root. I agree this part do not correspond the threading topic at all. It is my fault this code leaked into proposal.

I agree the positive logic is easier to understand. I think the reason I wrote it negative is a custom to write include guard in C-headers.

I absolutely agree about the cmake include options. The center question for me in here is whether to add libRTI directory. If you think that it is an appropriate build solution, I am ready to modify the patch in a corresponding way.

The polling I used is also an attempt to set aside the problem I know how to solve in favor of the those I do not. In general I had to deal with the internal message protocol between RTIA and libRTI. The standard approach is to remove "select" and add the third thread to get usually more efficient related waiting as a result. If you have the similar feeling I can have a look on it either.

I am also consider the ring buffer to be a good approach, however currently it seems to be the second place task. In my opinion, we should made a working patch first. Thus the straightforward, clear and reliable boost mutex-guard implementation is the current best.

Looking for the following comments,
Eugene

Eugene Chemeritskiy <tyz>
Group Member
Thu 16 Aug 2012 10:14:14 AM UTC, comment #3: 

Hi Eugene,

I did have a more thorough look to your patch.
First can you make the
OPTION(BUILD_TESTS ...OFF)
addition a separate patch.
By the way why wouldn't you want to build the test,
at least the default should tend to backward compatibility
with i.e. "ON" and not "OFF".
Anyway I think it is a separate issue.

A small nick-pick.
I'd rather use "positive" logic when it's possible.
i.e.

#ifndef RTIA_USE_THREAD
  blah
#else
  foo
#endif

should be easier to understand like:
#ifdef RTIA_USE_THREAD
  foo
#else
  blah
#endif

This are the same but it's usually easier to catch
"I'm in this case" than "when not in this case".


This
#include "../libRTIA/librtia_buf.hpp"
is not a good idea plkease use
#include "librtia_buf.hpp"
and add appropriate
set_target_properties(TheTarget PROPERTIES INCLUDE_DIRECTORIES "${CMAKE_SOURCE_DIRECTORY}/libRTIA")

or use CMake "include_directories" command.

The usage of Boost lock, guard etc... for libRTIABuf
seems like overkill. I think a mere in-memory ringbuffer
should be enough since there is only one reader and one writer
which makes ring buffer a lock-free data structure of choice
in this case.

The main pb with this is the polling mechanism you have to
do in order to replace the "select" you lost because you
don't have socket any more.

I shall stop the review here.
I'll continue tonight or tomorrow.



Eric NOULARD <erk>
Group administrator
Fri 20 Jan 2012 09:32:49 AM UTC, comment #2: 

Hi Eric,

Thank you for your fast and detailed reply.

Answering your first question, I have tested proposed patch on two tests. Each test federations consists of message [interaction] sender and registrar. Sender creates series of messages containing single numerical parameter. The initial positive value is set to parameter during the test initialization, and is decremented with each message send until it reaches zero value. During the first test the registrar just observes the passed parameter values. In case of the second test the registrar also replies to each message with its full copy,
and the sender does not send new messages until it gets the sender reply.
In fact, the second described test is a ping-pong test, I've submitted earlier: https://savannah.nongnu.org/patch/?7487.
I later learned, that despite the simplicity of these tests, they were used in some reserch papers and it has a consideration behind :-):
http://www.pitch.se/images//03s-siw-055.pdf [RTI-NGv6 Benchmarks].

Considering the benchmarking, for this purpose I have some python
scripts based on Dtest framework you adviced me before and give some automation. Actually comprehensive benchmarking of the proposed patch in comparison with the traditional CERTI architecture is currently my primary goal in this project.
I already have some unreliable preliminary results I can share:
Test No  WO Threads    Threads
1        4246 ms       1822 ms
2        8512 ms       4166 ms
This results I've got using a my laptop under 64 bit linux while 10 000 messages passed. These results seem to be rather prospective but now I'm not sure about the same dynamics
while using several computers - network communication may be the bottleneck here.

Considering your second question, primary target architecture is linux, but I am looking for a cross-platform solution in the first place.

Eugene Chemeritskiy <tyz>
Group Member
Fri 20 Jan 2012 07:22:28 AM UTC, comment #1: 

Hi Eugene,

This looks like a nice piece of patch, thanks.
I'll review it thoroughly but from what you said I may
already gives some remarks:

1) Making the multi-threaded compilation
   an option is definitely the way to (and keep)
   CERTI has (and may have again) some secure simulation
   application. In such application you cannot trust
   the federate, thus having the RTIA process as an insulation
   layer between the federate and RTI is necessary
   (in this case)

2) We try hard not to make CERTI source code dependent
   of third party lib. Of course Boost is very nice and
   effective library but depending on it may already be
   too much.

3) Concerning the two-sided activity of RTIA
   (RTIA side vs RTIG side) I would be tempted by
   3 threads approach (Federate, Fed:RTIA, RTIA:RTIG)
   which could generically be named Fed:LRC, LRC:CRC)

   Those threads may share some in-memory ring buffer
   and/or message queue.
   That said the difficult patr here is that part of our
   messaging protocol assume that some RTIA<-->RTIG
   message sequence may not be interleaved with federate<-->RTIA
   messages.
   This is easy to do now but may lead to subtle race
   when going multi-thread.

4) A student (Jean-Baptiste)
   Had begun a work on shared memory (without thread)
   this is unfinished work (and need good clean-up to)
   but you can have a look at
   libHLA/Semaphore* libHLA/SHM* classes.

5) I think the message based approach between
   Federate and RTI(A) should be kept
   but we must find a way to avoid multiple copy
   in the shared memory case.

Some questions:
Q1) Do you have any [automated] testcase that may
   be used to assert the performance gain?
    If yes did you try it with your patched version?
    If no I think we will really need this, in order
    to share a common testcase for the various
    platform supported by CERTI.

Q2) Which host platform do you target primiraly?
    Windows, Linux, ...

That my preliminary remarks.
All that said,
I'm really interested in making CERTI multithreaded
and I'm glad you started this.

More comment on the patch itself in a couple of day.

Eric NOULARD <erk>
Group administrator
Fri 20 Jan 2012 05:56:00 AM UTC, original submission:  

This patch slightly changes CERTI RTI process model in attempt to increase overall RTI performance. Currently local RTI component consists of two full-weight processes, namely RTIA and Federate process. These processes interact to each other by means of TCP or UNIX socket (UNIX pipe). This type of interaction requires a lot of repeated message representation transitions and seems not to be an optimal one. There are a lot of suitable approaches: for example using of shared memory can partially solve the issue. The same common address space benefit could be reached by means of multi-threaded program.
Thus patch replaces two processes of local RTI component by the single two-threaded process.

Patch includes minor architectural changes only. It leaves unchanged the existing logical separation of duties between processes and uses the same message-based approach for local on host synchronization. Subsequently these mechanisms should be replaced due to its ineffectiveness, but all in good time.
For example, it seems prospective to make federate callbacks directly in context of the RTIA thread.

The next issue that deserves some consideration is a simultaneous polling of Federate and RTIG in RTIA thread.
Proposed patch cope with this using a handicraft combination of non-blocking 'select()' and thread polling. This approach is definitely not the best one and requires additional effort. Common pattern solution in this situation is to use an additional (the third) thread dedicated to socket listening. An interesting direction of researches here is the possibility of concurrent handling of both RTIG and federate messages.

Technically patch adds an option 'RTIA_USE_THREAD' that allows building of multi-threaded architecture whereas the traditional multi-process architecture is still available by default. Multi-threading is reached using cross-platform boost::thread library.

There are a lot of issues to be solved, and I will be glad to discuss the proposal, the possibility of its integration into the main CERTI development branch, and the prospective of the further development in the specified direction.

Eugene Chemeritskiy <tyz>
Group Member

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files
file #24848:  thread_patch.diff added by tyz (130KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by erk (Posted a comment)
  • -email is unavailable- added by tyz (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 5 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2015-04-02 erk StatusIn Progress Postponed
        Assigned toerk None
    2012-01-20 erk StatusNone In Progress
        Assigned toNone erk
    2012-01-20 tyz Attached File- Added thread_patch.diff, #24848

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code