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 Fri, 29 Sep 2006 09:41:11 GMT
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?

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



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

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

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


Mime
View raw message