harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Andrey Chernyshev" <a.y.chernys...@gmail.com>
Subject Re: [drlvm] HARMONY-1582 - invocation API for DRLVM
Date Fri, 29 Sep 2006 19:34:44 GMT
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?

I would expect TM be responsible for launching  threads, and VM be
responsible for creating JNIEnv for each new thread. VM may have a
method like "create_JNIEnv()" which TM might be using while building a
new Java thread (or attaching the existing native one). We can extend
the existing vm_attach() function for that purpose for now, such that
it would be returning new JNIenv's.

>
>
> >
> > (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.

Looking into the patch, it really seems like you just added a
convenience method which does an allocation of the hythread_lib_t
structure with help of APR. How does that help to solve the multiVM
issue? hythread_lib was a variable in vm_main in the past, but what
prevents you from just making  it a part of the appropriate JavaVM
struct?


>
> >
> > (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?

OK, then why don't suggest just to rename the hythread_global_lock to
the hythread_lib_lock if you feel this is that important?  I guess
this must be more safe if we were introducing one more global lock to
the system.

>
> >
> > (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...

Right, it isn't good, we should better think how the daemon-related
functionality could be moved to the Java level.

Thanks,
Andrey.

>
> 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
>
>


-- 
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


Mime
View raw message