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 CHECKPOINT
Date Mon, 02 Oct 2006 09:40:39 GMT
Andrey,

Just to be clear.... I agree with you it is more convenient if
jthread_create takes JNIEnv instead of JavaVM. It reflects that
current thread has been attached already. Do you think it makes sense
to get rid of JNIEnv and use jthread_get_JNI_env in that case?
Regarding jthread_attach. I still didn't get your point.... Clarify it
please if you think jhread_attach should be modified.

Thank you
Evgueni

On 10/2/06, Evgueni Brevnov <evgueni.brevnov@gmail.com> wrote:
> On 9/29/06, Andrey Chernyshev <a.y.chernyshev@gmail.com> wrote:
> > On 9/29/06, Evgueni Brevnov <evgueni.brevnov@gmail.com> wrote:
> > > Artem,
> > >
> > > Thank you for your feedback.... find my inlined.....
> > >
> > > On 9/29/06, Artem Aliev <artem.aliev@gmail.com> wrote:
> > > > Evgueni,
> > > >
> > > > I got most of your changes, but still disagree with all
> > > > hythread/jthread interface changes. Could leave interface unchanged.
> > > > See following possible solutions, that could solve the same problems
> > > > without interface changes.
> > > >
> > > >
> > > > 1) daemon attribute is a java specific. (Andrey mentioned this already).
> > > >   Could you please move it back. to the jthread layer. It is better
> > > > to rename
> > > >   hythread_wait_for_all_nondaemon_threads() to
> > > > jthread_wait_for_all_nondaemon_threads().
> > > Ok, I see no problems to move "daemon" to java layer. In that case:
> > > 1) I will move hythread_wait_for_all_nondaemon_threads() from
> > > thread_init.c to one which implements java layer.
> > > 2) I will move daemon field from HyThread structure.
> > >
> > > Agree?
> >
> > Sounds good to me.
>
> OK, will do that.
>
> >
> >
> > >
> > > >
> > > > 2)  JavaVM could be retrieved  from JNIEnv by  jni_get_java_vm(). So
> > > > let the jthread_create() and others to use JNIEnv (that is passed from
> > > > java native method).
> > > > The vm_attach could get old JNI env and create new one for the new thread.
> > > > The first JNIEnv is created in CreateVM call and could be passed to
> > > > the first thread at startup.
> > > No, no, no. I completely disagree with that!!! Why do you like JNIEnv
> > > but JavaVM. Why do you think that passing JavaVM instead of JNIEnv
> > > makes TM less modular? I don't see any difference here.... It seems
> > > for me like a big big hack to grab JNIEnv from another thread and pass
> > > it to jthread_attach to perform operations in the current thread.
> >
> > TM needs to know JNIEnv, mainly for managing the references to
> > objects, throwing exceptions and calling run() method of a new thread.
> > JNI spec proposes that JNIEnv is valid within the given thread, this
> > is why TM holds the JNIEnv pointer at the moment. This is why TM likes
> > the JNIEnv.
> >
> > Having the JNIEnv, one is able to get JavaVM but not vise versa. This
> > is why TM doesn't like the JavaVM :)
> I see your point. Only one note this is true for already attached threads...
>
> >
> > I agree with you that there is a design flaw that the JNIEnv is copied
> > from the parent thread to a child thread during thread creation. I
> > think it could be resolved via vm_attach() hook - with this event, VM
> > might tell the TM what the JNIEnv pointer for new thread should be. I
> > think you did that by extending the vm_attach() call with the JNIEnv**
> > argument.
> >
> > What is not completely clear is, why do you have to pass the JavaVM
> > forth and back, once from VM to TM, and then back from TM to VM again?
> >
> > If you need to know in jthread_attach, which particular VM vm_attach()
> > event is coming from, you could have vm_attach like
> > vm_attach(JNIEnv* currentThreadEnv,  JNIEnv** newThreadEnv).
> I'm a little bit confused.....Current thread hasn't been attached yet.
> So there is no JNIEnv for it yet. How it can be passed as the first
> parameter to vm_attach()?
>
> > Then you will be always able to get the JavaVM from the JNIEnv.
> > The only difference is that you are currently doing JNIEnv->JavaVM
> > conversion in the VMThreadManager, but why can't you just do this in
> > vm_attach() without changing the interface of the TM?
> > So far JavaVM really looks like an extra knowledge that TM doesn't
> > have to be aware of.
> >
> > > Moreover there is no JNIEnv when main thread attaches to VM. So we
> > > should either keep it as is or change original design of TM and attach
> > > thread to VM before attaching it to TM. In that case we will have
> > > valid JNIEnv which can be passed to jthread_atatch. We need to think
> > > over it twice before changing something....
> >
> > Right. For jthread_attach, JNIenv needs to be changed from being input
> > parameter to being the output parameter. The way how JNIenv is
> > obtained by TM should be vm_attach() event.
> OK, JNIEnv is output parameter. And it obtained by vm_attach(). This
> is exactly like I do in the patch. What I don't understand is how
> jthread_attach knows to which VM the thread should be attached? Do you
> suggest calling vm_attach first to create JNIEnv it pass it to
> jthread_attach?
>
> >
> > >
> > >
> > >
> > > >
> > > >
> > > > 4) Original classlib hythread do not use hythread_library_t in arguments,
> > > > It uses following code:
> > > >
> > > >  hythread_library_t lib = GLOBAL_DATA (default_library);
> > > > or
> > > >  hythread_library_t library = thread->library;
> > > >
> > > > So could you please use the same mechanism and remove hythread_*_ex >functions.
> > > Let's see if classlib's hythread needs changing first. It seems for me
> > > such code prevents us from having multi VM.
> > >
> > > >
> > > > 5. You introduce more then one jni env, but still use global variable
for it.
> > > > So all changes like following:
> > > > -    JNIEnv *jenv = (JNIEnv*)jni_native_intf;
> > > > +    JNIEnv *jenv = jni_native_intf;
> > > >
> > > > should be like:
> > > > -    JNIEnv *jenv = (JNIEnv*)jni_native_intf;
> > > > +    JNIEnv *jenv = get_jni_env(jthread_self());
> > >
> > > Ok, I see. I agree that global jni_native_intf should not be used.
> > > There was simple reason why I altered such lines. Because I changed
> > > the type of  jni_native_intf and no casting operator is needed now. To
> > > be honest I think get_jni_env(jthread_self()) can be good as temporary
> > > solution only. Lets wait for design of multi VM and fix it according
> > > to it.
> >
> > While we are in JNI code, we always have the JNIenv (at least
> > initially it comes from Java code). If we consider VM code as if it
> > was a JNI application, then it seems like we should be just passing
> > JNIEnv as a parameter to all functions in VM. Or, we can be taking it
> > from TLS (via jthread_self()), depending on which way is faster...
> Agree.
>
> >
> > >
> > > >
> > > > 6). And small remarks:
> > > > +jint vm_init1(JavaVM_Internal * java_vm, JavaVMInitArgs * vm_arguments);
> > > > +jint vm_init2(JNIEnv_Internal * jni_env);
> > > > Could you make names more meaningful, then 1,2,3...?
> > > Ok, will do that.
> > >
> > > >
> > > > class VM_thread {
> > > > ...
> > > > +    JNIEnv_Internal * jni_env;
> > > > The jthread already has the jni_env pointer, you do not need to
> > > > duplicate it here.
> > > > forexample by using jthread_get_JNI_env(jthread_self());
> > >
> > > Yes I know. I don't see any problems here. Some times it is much more
> > > convenient to get JNIEnv from VM_thread structure (and faster) instead
> > > of doing jthread_get_JNI_env(jthread_self()). So I need strong
> > > arguments for removing it. Again it seems that should be addressed in
> > > design of multi VM. So lets forget about it for a while...
> >
> > I think that the data duplication would always serve as a potential
> > source of errors - while updating one copy of object, you may forget
> > to update the other, often resulting into a strange behavior of the
> > whole application. Let's see what are the specific performance
> > concerns that have to be addressed. To get VM_thread structure, you
> > would eventually go to the TLS, just like
> > jthread_get_JNI_env(jthread_self() would do.
> If there is already VM_thread structure for some reasons then there
> will be no extra access to TLS. It is definitely much more in
> jthread_get_JNI_env(jthread_self() than just one TLS access and one
> dereferncing. I don't think it is a really big problem now. Do you
> agree to look at this later. I guess multi VM implementation will
> alter it in any case.
>
> Thanks
> Evgueni
>
> >
> > Thanks,
> > Andrey.
> >
> > >
> > > Evgueni
> > > >
> > > > Thanks
> > > > Artem
> > > >
> > > > On 9/28/06, Evgueni Brevnov <evgueni.brevnov@gmail.com> wrote:
> > > > > I suppose two days silence means that there is no objects (maybe
> > > > > interest) against proposed patch. I would suggest to commit it ASAP.
> > > > > It really works! There are some cases when current VM crashes but
the
> > > > > patch fixes it. I can work on bringing cunit tests to live as soon
as
> > > > > the patch is committed.... This is just my understanding.
> > > > >
> > > > > Thanks
> > > > > Evgueni
> > > > >
> > > > > On 9/28/06, Geir Magnusson Jr. <geir@pobox.com> wrote:
> > > > > > So where are we here?
> > > > > >
> > > > > > On Sep 28, 2006, at 12:41 AM, Evgueni Brevnov wrote:
> > > > > >
> > > > > > > On 9/28/06, Weldon Washburn <weldonwjw@gmail.com>
wrote:
> > > > > > >> On 9/26/06, Evgueni Brevnov <evgueni.brevnov@gmail.com>
wrote:
> > > > > > >> >
> > > > > > >> > On 9/27/06, Andrey Chernyshev <a.y.chernyshev@gmail.com>
wrote:
> > > > > > >> > > (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?
> > > > > > >>
> > > > > > >>
> > > > > > >> Well,  this sort of, kind of sounds right but not quite.
 Its a
> > > > > > >> little more
> > > > > > >> subtle than being compatible with IBM threading library.
 The
> > > > > > >> first goal is
> > > > > > >> to identify the parts of IBM threading library that
are JVM
> > > > > > >> independent.  It
> > > > > > >> makes sense for DRLVM to be compatible with the independent
> > > > > > >> parts.   This
> > > > > > >> should be a nobrainer.
> > > > > > >>
> > > > > > >> The parts of IBM threading library that assume a specific
JVM
> > > > > > >> implementation
> > > > > > >> will be a problem.  We will need to find a solution
that is
> > > > > > >> endorsed by all
> > > > > > >> the stakeholders (including J9 folks).  The hythread_global_lock
> > > > > > >> falls into
> > > > > > >> this category.  For starts, I would like to see a concise
> > > > > > >> description from
> > > > > > >> the portlib owners on what hythread_global_lock protects,
which
> > > > > > >> locks have
> > > > > > >> to be held before grabbing this lock, are there any
restrictions
> > > > > > >> on what
> > > > > > >> system calls can be made while holding this lock (like
sleep or
> > > > > > >> wait), etc.
> > > > > > >
> > > > > > > Weldon, I completely agree with what your are saying. It's
common
> > > > > > > problem of current hythread that should be resolved some
how. I just
> > > > > > > go inline with current implementation and added two missing
functions.
> > > > > > > Missing these can lead to the same problems as with hythread_exit
> > > > > > > discussed  in another thread "[drlvm] [launcher] Executable
hangs".
> > > > > > >
> > > > > > >>
> > > > > > >> To get a better idea what's in the patch.diff, I printed
it out.
> > > > > > >> Its 120+
> > > > > > >> pages.  Quite a big patch!  Most of it looks like straight
forward
> > > > > > >> JNI
> > > > > > >> interface glue.  There are some tricky parts.  I would
like to
> > > > > > >> know the
> > > > > > >> design review process for these parts.  Using grep,
I found 20
> > > > > > >> locations
> > > > > > >> where ...suspend_enable... and ...suspend_disable...
have been
> > > > > > >> added.  And
> > > > > > >> 25 locations where enable/disable have been removed.
 Failure in
> > > > > > >> this logic
> > > > > > >> can lead to incorrect reference pointer enumeration.
 These are
> > > > > > >> probably the
> > > > > > >> hardest bugs to find.  Please tell us who has looked
at this code
> > > > > > >> in depth.
> > > > > > > Only me and you :-) Honetsly I think it happpens now....
> > > > > > >
> > > > > > >> Are there any known design flaws in it?
> > > > > > > I can think of two possible problems we may want to discuss.
> > > > > > > 1) Should native threads have "daemon" status or its completely
java
> > > > > > > notion? This is TM related thing.
> > > > > > > 2) Should we attach thread to VM before attaching it to
TM by calling
> > > > > > > jthread_atatch OR jthread_attach should callback VM to
attach a thread
> > > > > > > to it? I didn't change original design of TM here ......
it implements
> > > > > > > second choice.
> > > > > > >
> > > > > > >>
> > > > > > >> I also notice APIs called tmn_suspend_enable(),
> > > > > > >> hythread_suspend_enable()
> > > > > > >> -- are these simply different names for the same binary
> > > > > > >> executible.  Or
> > > > > > >> different binaries that do the same thing??
> > > > > > >
> > > > > > > No, this is not just different names. tm_suspend_enable
asserts that
> > > > > > > thread is in disabled state before calling hythread_suspend_enable
(in
> > > > > > > debug mode only).
> > > > > > >
> > > > > > > Thanks
> > > > > > > Evgueni
> > > > > > >>
> > > > > > >>
> > > > > > >> --
> > > > > > >> > Weldon Washburn
> > > > > > >> > 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
> > > > > >
> > > > > >
> > > > >
> > > > > ---------------------------------------------------------------------
> > > > > 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