On 10/3/06, Evgueni Brevnov <[EMAIL PROTECTED]> wrote:
On 10/3/06, Andrey Chernyshev <[EMAIL PROTECTED]> wrote:
> On 10/2/06, Evgueni Brevnov <[EMAIL PROTECTED]> wrote:
> > 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?
>
> The jthread_* layer is designed like if it were a regular JNI
> application which is meant to be called from the Java code, hence
> every function on that layer receives JNIenv. We can probably get rid
> of the JNEnv parameter for jthread_* functions, assuming that TM can
> always extract JNIenv for the current thread with
> jthread_get_JNI_env().
> My only concern would be the performance - once the JNIenv is already
> known for the native part of the kernel classes impl, it must be
> cheaper to pass JNIEnv to TM as an extra parameter rather than extract
> it from the TLS again.
> Other than that, I see no strong advantages in keeping JNIEnv parameter.
>
>
> > Regarding jthread_attach. I still didn't get your point.... Clarify it
> > please if you think jhread_attach should be modified.
>
> Sorry for being not clear: I think for jthread_attach, we have two options:
> 1) Make JNIEnv input parameter - it must be new JNIEnv that VM
> pre-allocates for the new Java thread. jthread_attach would just
> associate it with the current thread.
>
> 2) Obtain JNIEnv via vm_attach() callback. In this case, if
> vm_attach() callback implementation needs to know for which JavaVM new
> JNIenv has to be allocated, then we'll need to add JavaVM as input
> parameter for jthread_attach().
> I think both options should be fine, (1) would probably keep TM
> interface a bit lighter, though (2) may look more closer to the JNI
> invocation API idea.
> So I think adding JavaVM specifically for jthread_attach may make
> sense given the explanation you provided earlier.
>
> The concern I would have regarding the proposed jthread_attach
> implementation is a call to vm_create_jthread() - this call introduces
> an extra dependency of TM on vmcore that I'd prefer to be avoided. In
> the original version, jthread_attach() was taking jthread argument of
> the already prepared j.l.Thread.
I understand your concern. Unfortunately I don't see what we can do
here. Let me explain. In the beginning you have an unattached native
thread. To be able to call java code (which is required for
constructing j.l.Thread instance) the thread should be attached first.
To be specific it should be attached to VM by calling vm_attach which
will return a valid JNIEnv It is valid to use JNI from that moment.
Obtained JNIEnv can be used to execute java code and create j.l.Thread
instance. Since we do vm_attach in jthread_attach we need to do
vm_create_jthread inside jthread_atach as well.
Of course it can be an alternative to do vm_attach and
vm_create_jthread outside of TM right before jthread_attach. Sure it
will reduce one dependence between VM and TM. But it seems like
artificial action for me just because of dependency....
Why do you think it is artificial? I would rather prefer not to throw
vm_attach event from the jthread_attach() call at all than to add
extra dependency. The idea of jthread layer is a Java face to
hythread, it is meant to know either a little or nothing about the
rest of VM. It may be logical to throw vm_attach call from the newly
created thread, because there is no other way to let know VM the new
thread is created. VM attach is a different case - VM already knows
about new Java thread at the time of the AttachCurrentThread call.
> Do you think it makes sense to replace a single jthread parameter with
> a variety of stuff (like thread group, name)? It seems the only
> purpose of at these args is to be passed back to VM for
> vm_jthread_create(). I would suggest to change this and try doing
> either of:
> 1) Make signature of jthread_attach with 3 args, JavaVM, jthread and the
daemon.
Why do you want to pass daemon to TM but thread's name and group. Just
because current TM doesn't need such information? What if it is
required to get thread name later? Say by introducing
I imagine you need a daemon attribute since the TM is still managing
the thread daemonality. TM is not managing thread name and group -
they are all kept on the Java level, hence passing them may be
excessive.
jthread_get_name... What will you do in that case? Let me guess you
will call j.l.Thread.getName. Right. Ok! In that case I see no
problems to call j.l.Thread.isDaemon. Do you agree? So it doesn't
As I suggested earlier, the best way to handle daemonality would
probably be in pure Java - in j.l.Thread (or j.l.VMThreadManager) or
whatever. You already lifted it up to the jthread level, but what if
we can go a little bit higher...
seems to be a good design to pass only part of the information to
jthread_atach. Lets look at the signature of AttachCurrentThread. It
takes exactly these three parameters (daemon, name, group) passed as a
structure. I was thinking about having exactly the same structure as
third parameter of jthread_attach but it occured to be more convinient
to pass them seperatly. Although I don't see strong reasons against
having a structure a third parameter.
I see. Acually, I would love to keep the jthread_attach func-ty at the
minimum level which will be needed to handle the only data that TM
should be aware of. We need a formal boundary between jthread layer
and vmcore (otherwise jthread won't have a much of sense, one may
consider it just as a convenience set of functions in vmcore which are
doing something with threading).
> 2) Move the implementation of vm_create_jtrhead() to
> thread_java_basic.c - could it be written in pure JNI without using
> internal VM API like class_alloc_new_object()?
Yes, this can be done but it doesn't fix the problem. You still need
to know about internal constructor of j.l.Thread
That's bad. Given what you said, now it seems that the most preferable
sequence for AttachCurrentThread impl still would be like:
JNIEnv = vm_attach();
jthread = create_jthread(JNIenv)
jthread_attach(JNIEnv, jthread) // stores JNIEnv and Hythread into
TLS, doesn't call vm_attach any longer.
- this is almost what we had from the beginning...
Thanks,
Andrey.
Thanks
Evgueni
>
>
> Thanks,
> Andrey.
>
> >
> > Thank you
> > Evgueni
> >
> > On 10/2/06, Evgueni Brevnov <[EMAIL PROTECTED]> wrote:
> > > On 9/29/06, Andrey Chernyshev <[EMAIL PROTECTED]> wrote:
> > > > On 9/29/06, Evgueni Brevnov <[EMAIL PROTECTED]> wrote:
> > > > > Artem,
> > > > >
> > > > > Thank you for your feedback.... find my inlined.....
> > > > >
> > > > > On 9/29/06, Artem Aliev <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]> 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. <[EMAIL PROTECTED]> wrote:
> > > > > > > > So where are we here?
> > > > > > > >
> > > > > > > > On Sep 28, 2006, at 12:41 AM, Evgueni Brevnov wrote:
> > > > > > > >
> > > > > > > > > On 9/28/06, Weldon Washburn <[EMAIL PROTECTED]> wrote:
> > > > > > > > >> On 9/26/06, Evgueni Brevnov <[EMAIL PROTECTED]> wrote:
> > > > > > > > >> >
> > > > > > > > >> > On 9/27/06, Andrey Chernyshev <[EMAIL PROTECTED]> 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: [EMAIL PROTECTED]
> > > > > > > > > For additional commands, e-mail: [EMAIL PROTECTED]
> > > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > >
---------------------------------------------------------------------
> > > > > > > > Terms of use : http://incubator.apache.org/harmony/mailing.html
> > > > > > > > To unsubscribe, e-mail: [EMAIL PROTECTED]
> > > > > > > > For additional commands, e-mail: [EMAIL PROTECTED]
> > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > > >
---------------------------------------------------------------------
> > > > > > > Terms of use : http://incubator.apache.org/harmony/mailing.html
> > > > > > > To unsubscribe, e-mail: [EMAIL PROTECTED]
> > > > > > > For additional commands, e-mail: [EMAIL PROTECTED]
> > > > > > >
> > > > > > >
> > > > > >
> > > > > >
---------------------------------------------------------------------
> > > > > > Terms of use : http://incubator.apache.org/harmony/mailing.html
> > > > > > To unsubscribe, e-mail: [EMAIL PROTECTED]
> > > > > > For additional commands, e-mail: [EMAIL PROTECTED]
> > > > > >
> > > > > >
> > > > >
> > > > > ---------------------------------------------------------------------
> > > > > Terms of use : http://incubator.apache.org/harmony/mailing.html
> > > > > To unsubscribe, e-mail: [EMAIL PROTECTED]
> > > > > For additional commands, e-mail: [EMAIL PROTECTED]
> > > > >
> > > > >
> > > >
> > > >
> > > > --
> > > > Andrey Chernyshev
> > > > Intel Middleware Products Division
> > > >
> > > > ---------------------------------------------------------------------
> > > > Terms of use : http://incubator.apache.org/harmony/mailing.html
> > > > To unsubscribe, e-mail: [EMAIL PROTECTED]
> > > > For additional commands, e-mail: [EMAIL PROTECTED]
> > > >
> > > >
> > >
> >
> > ---------------------------------------------------------------------
> > Terms of use : http://incubator.apache.org/harmony/mailing.html
> > To unsubscribe, e-mail: [EMAIL PROTECTED]
> > For additional commands, e-mail: [EMAIL PROTECTED]
> >
> >
>
>
> --
> Andrey Chernyshev
> Intel Middleware Products Division
>
> ---------------------------------------------------------------------
> Terms of use : http://incubator.apache.org/harmony/mailing.html
> To unsubscribe, e-mail: [EMAIL PROTECTED]
> For additional commands, e-mail: [EMAIL PROTECTED]
>
>
---------------------------------------------------------------------
Terms of use : http://incubator.apache.org/harmony/mailing.html
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
--
Andrey Chernyshev
Intel Middleware Products Division
---------------------------------------------------------------------
Terms of use : http://incubator.apache.org/harmony/mailing.html
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
