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 Mon, 02 Oct 2006 04:47:42 GMT
On 9/30/06, Andrey Chernyshev <a.y.chernyshev@gmail.com> wrote:
> 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.

I did exactly like you suggest here :-) Please look at vm_jthread_atach.

>
> >
> >
> > >
> > > (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?
Yes, I did it. If you look at Global_Env structure you will find it
have a pointer to portlib.

>
>
> >
> > >
> > > (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.
I agree with you. There is no need to keep both. I just left former
one for not introducing aaditional changes in this patch. Do you
suggest to include that in this patch or do it separatly?

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

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

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