Ok here is some details on threading library changes. Most of changes in TM affects two files thread_native_basic.c and thread_java_basic.c. Lets start with the first one. Main cause of all modification in that file comes from refactoring hythread_attach_to_group and hythread_detach functions.
1) hythread_attach_to_group a) new hythread_library_t parameter was added to the function signature. What for? For multi VM :-) When you attaches a thread to a thread library you need to specify to which particular library because each VM has its own. b) I've changed initialization/allocation of HyThread structure for attaching thread. Why? Bug in the current implementation. Input parameter (handle) is processed incorrectly. New instance of HyThread structure is created each time instead of using *handle one. This is expected behaviour according to specification of hythread_atach. So it is the bug. c) Setting TLS and changing thread status to alive and runnable is encapsulated in one place register_to_group. I will talk about it later. 2)hythread_detach a) originally there were two functions public hythread_detach and private destroy_thread. Each of them did its part of job when thread is not needed any more. I just merged them into one to make it easy to use and understand. Why? If you look closer to original hythread_detach it calls thread_destroy. So thread_destroy is a part of detaching. Fine. Lets jump to thread_start_proc. Look at shutdown sequence. It calls thread_destroy and sets TLS to NULL. Hmm this sequence does exactly the same as hythread_detach. Lets call hythread_detach instead. OK. Then what is the reason to have thread_destroy? When should we use thread_destroy instead of hythread_detach. I suspect never. If we do so we are asking for troubles. What I did? I concentrate all that stuff in one place. It is much linear I believe. 3) reset_thread, init_thread. There is a common problem with these two functions. They both add HyThread strucure to the specified thread group. It really bad. There are use cases when reset_thread called several times before a real thread is forked. In that case the same thread will be added to the group several times. Moreover reset_thread adds the thread to the same group each time what if I want to reuse HyThread structure but register it to another group. Ups doesn't work. So I moved group processing out of these functions. The last thing here is changing function name from init_thread to allocate_thread because it really allocates new HyThread structure. I don't insist on renaming but it seems new name reflects things better. 4) allocate_thread. I'm really confused by such names. There is init_thread that allocates new HyThread structure. And allocate_thread which adds allocated thread to the group. Sorry but I renamed it to allocate_thread and register_to_group correspondingly. In the current implementation code which deals with registering a thread to thread group is spread among three functions reset_thread, init_thread, allocate_thread... I put all that code in one place register_to_group and call when necessary. Seems to be easy.... That's all about hythread_native_basic.c. I need to leave my computer for a couple hours. I will come back with the second part.... Evgueni On 9/27/06, Evgueni Brevnov <[EMAIL PROTECTED]> wrote:
On 9/27/06, Andrey Chernyshev <[EMAIL PROTECTED]> wrote: > On 9/26/06, Evgueni Brevnov <[EMAIL PROTECTED]> wrote: > > >Alexey Varlamov [26/Sep/06 05:11 AM] > > >Evgueni, thank you, quite impressive work! > > >Unfortunately patch is so huge it is hard to understand at once. > > > > I understand your concern (about reformatting)... I feel very-very > > uncomfortable when I study purely formatted code. It always hard to > > understand some one's code especially if it is hard to read. That was > > not my intention to fix indenting I just can't study such code and > > reformat it on the fly.... Sorry, I don't know how to switch my patch > > to the original formatting. > > > > >And what really bothers, is that about half of it is just reformatting :( > > >Can't we really go without white space changes and renaming of > > >parameters\local vars??? > > Its definitely much less than the half of the patch. > > > > > > > >Kind request: could you please describe shortly what is done in TM - > > which >essential changes & enhancements? > > > > I think a lot of people are interested in answer to this question. Do > > you? Let me get a break and I will come up with details.... > > Right, it seems like invocation API patch does extensive changes to > the TM design and interfaces. Hopefully you can provide us with a > summary why it was done > so. Sure! > In the meantime, there are a few things I'd like to understand > first (I didn't > look through the whole patch yet though): > > (1) > Why did you have to add JavaVM to jthread_create and jthread_attach > functions? I couldn't see the use for JavaVM in the TM code, except putting it > into TLS which can always be done outside of TM if one wishes to do so. > Ideally, we would have TM to know as much less about the rest of VM as >possible. Mainly because I'm looking forward to have multi VM in a process address space. So I tried to do everything having this in mind. Take jthread_attach function. Originally it had JNIEnv * as its first argument. According to JNI spec JNIEnv * pointer is valid in the current thread only! Before thread is not attached it doesn't have its environment. So it can be a parameter to jthread_attach function. To be honest I go iniline with original design here.... VM calls jthread_attach to attach thread....jthread_attach calls vm_attach (vm_jthread_attach now). We may think about changing that to the following. VM attaches thread to it self by means of vm_jthread_attach and then passes created JNIEnv * pointer to jthread_attach. What do you think? > > (2) > What is the purpose of adding hythread_lib_create and > hythread_lib_destroy functions? I guess we already have > hythread_init/hythread_shutdown for the similar purpose... Multi VM again. Each VM should have its own instance of the library. > > (3) > One more lock is added - hythread_lib_lock. How is that differ from > the hythread_global_lock that we already have? Each extra lock to the > system may add more possibilities for deadlocks, as well as can > negatively impact the scalability (unless some of the existing locks > are split). hythread_lib_lock acquires exactly the same lock as hythread_global_lock. Probably I miss something but we need to be compatible with IBM threading library now. This library has such function. That's why I added it. Sounds right? > > (4) > Instead of adding "daemon" attribute to hythread_create, I think the > better approach would be to move the daemon threads count logic out of > the hythread level - this level is assumed to know nothing about > daemon threads in Java, the better place to handle that would be > jthread level (or even better - Java code). > Did you need this change to implement the invocation API impl, or, it > was just a side improvement? When I first looked at that I thought exactly like you. "daemon" notion should be applied to java threads only. But I found that you have daemon attribute into HyThread structure. Moreover you have hythread_wait_for_all_daemon_threads which get me the feeling that any hythread can be a daemon. I think for a while whether it is good or not to have such notion for native threads.....and decides it can be useful sometimes. Current implementation of InvocationAPI uses daemon status for java threads only. So lets discuss it... Thanks Evgueni > > Thanks, > Andrey. > > > > > > On 9/26/06, Evgueni Brevnov <[EMAIL PROTECTED]> wrote: > > > On 9/26/06, Geir Magnusson Jr. <[EMAIL PROTECTED]> wrote: > > > > > > > > On Sep 26, 2006, at 9:39 AM, Evgueni Brevnov wrote: > > > > > > > > > On 9/26/06, Geir Magnusson Jr. <[EMAIL PROTECTED]> wrote: > > > > >> Wanted to start a new thread with better subject line. > > > > >> > > > > >> Evgueni... two things: > > > > >> > > > > >> 1) by your own admission, there are problems with the patch, namely > > > > >> tests failing. > > > > > I do belive this is not the patch problems. I think this patch > > > > > disclose existing problems with condition variables implementation. > > > > > > > > > >> You also note that c-unit tests have to be turned off > > > > >> because they no longer link. > > > > > There is a hack in current cunit tests. Two methods are stubed > > > > > vm_attach and vm_detach. That's who linking problem is workarounded. > > > > > We can't go forward with this.... > > > > > > > > Who can we badger into fixing this? > > > > > > > > > > > > > >> > > > > >> 2) Alexey V is hoping for a description of the changes, due to the > > > > >> size and mix of reformatting changes. > > > > >> > > > > >> I don't mind a little breakage to move forward, but as we're just > > > > >> getting DRLVM stable after the launcher change, it might be better if > > > > >> we don't have to remove major things like c-unit tests to make that > > > > >> forward movement. > > > > > I believe it is a question of one day or even less to get it fixed by > > > > > original test authors. Unfortunately, I'm not familiar with cunit > > > > > tests internals. Ofcourse I can fix it by myself but need a little bit > > > > > more time.... > > > > > > > > Lets see if we can shame them into fixing it... :) > > > > > > > > Shame c-unit test authors! Shame c-unit test authors! > > > > > > Ok, I'm expecting a lot of anger on me form c-unit test authors ... > > > :-) Sorry, guys... > > > > > > > > > > > (goes off to look for patch...) > > > > > > > > geir > > > > > > > > > > > > > > Evgueni > > > > >> > > > > >> geir > > > > >> > > > > >> > > > > >> --------------------------------------------------------------------- > > > > >> Terms of use : http://incubator.apache.org/harmony/mailing.html > > > > >> To unsubscribe, e-mail: [EMAIL PROTECTED] > > > > >> For additional commands, e-mail: harmony-dev- > > > > >> [EMAIL PROTECTED] > > > > >> > > > > >> > > > > > > > > > > --------------------------------------------------------------------- > > > > > Terms of use : http://incubator.apache.org/harmony/mailing.html > > > > > To unsubscribe, e-mail: [EMAIL PROTECTED] > > > > > For additional commands, e-mail: [EMAIL PROTECTED] > > > > > > > > > > > > > > > > > --------------------------------------------------------------------- > > > > Terms of use : http://incubator.apache.org/harmony/mailing.html > > > > To unsubscribe, e-mail: [EMAIL PROTECTED] > > > > For additional commands, e-mail: [EMAIL PROTECTED] > > > > > > > > > > > > > > > --------------------------------------------------------------------- > > Terms of use : http://incubator.apache.org/harmony/mailing.html > > To unsubscribe, e-mail: [EMAIL PROTECTED] > > For additional commands, e-mail: [EMAIL PROTECTED] > > > > > > > -- > Andrey Chernyshev > Intel Middleware Products Division > > --------------------------------------------------------------------- > Terms of use : http://incubator.apache.org/harmony/mailing.html > To unsubscribe, e-mail: [EMAIL PROTECTED] > For additional commands, e-mail: [EMAIL PROTECTED] > >
--------------------------------------------------------------------- Terms of use : http://incubator.apache.org/harmony/mailing.html To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
