harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Evgueni Brevnov" <evgueni.brev...@gmail.com>
Subject Re: [drlvm] HARMONY-1582 - invocation API for DRLVM
Date Wed, 27 Sep 2006 07:54:32 GMT
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 <evgueni.brevnov@gmail.com> wrote:
> On 9/27/06, Andrey Chernyshev <a.y.chernyshev@gmail.com> wrote:
> > On 9/26/06, Evgueni Brevnov <evgueni.brevnov@gmail.com> 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 <evgueni.brevnov@gmail.com> wrote:
> > > > On 9/26/06, Geir Magnusson Jr. <geir@pobox.com> wrote:
> > > > >
> > > > > On Sep 26, 2006, at 9:39 AM, Evgueni Brevnov wrote:
> > > > >
> > > > > > On 9/26/06, Geir Magnusson Jr. <geir@pobox.com> 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: harmony-dev-unsubscribe@incubator.apache.org
> > > > > >> For additional commands, e-mail: harmony-dev-
> > > > > >> help@incubator.apache.org
> > > > > >>
> > > > > >>
> > > > > >
> > > > > > ---------------------------------------------------------------------
> > > > > > Terms of use : http://incubator.apache.org/harmony/mailing.html
> > > > > > To unsubscribe, e-mail: harmony-dev-unsubscribe@incubator.apache.org
> > > > > > For additional commands, e-mail: harmony-dev-help@incubator.apache.org
> > > > > >
> > > > >
> > > > >
> > > > > ---------------------------------------------------------------------
> > > > > Terms of use : http://incubator.apache.org/harmony/mailing.html
> > > > > To unsubscribe, e-mail: harmony-dev-unsubscribe@incubator.apache.org
> > > > > For additional commands, e-mail: harmony-dev-help@incubator.apache.org
> > > > >
> > > > >
> > > >
> > >
> > > ---------------------------------------------------------------------
> > > Terms of use : http://incubator.apache.org/harmony/mailing.html
> > > To unsubscribe, e-mail: harmony-dev-unsubscribe@incubator.apache.org
> > > For additional commands, e-mail: harmony-dev-help@incubator.apache.org
> > >
> > >
> >
> >
> > --
> > Andrey Chernyshev
> > Intel Middleware Products Division
> >
> > ---------------------------------------------------------------------
> > Terms of use : http://incubator.apache.org/harmony/mailing.html
> > To unsubscribe, e-mail: harmony-dev-unsubscribe@incubator.apache.org
> > For additional commands, e-mail: harmony-dev-help@incubator.apache.org
> >
> >
>

---------------------------------------------------------------------
Terms of use : http://incubator.apache.org/harmony/mailing.html
To unsubscribe, e-mail: harmony-dev-unsubscribe@incubator.apache.org
For additional commands, e-mail: harmony-dev-help@incubator.apache.org


Mime
View raw message