On 9/27/06, Evgueni Brevnov <[EMAIL PROTECTED]> wrote:
On 9/27/06, Andrey Chernyshev <[EMAIL PROTECTED]> wrote:
> On 9/26/06, Evgueni Brevnov <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]> wrote:
> > > On 9/26/06, Geir Magnusson Jr. <[EMAIL PROTECTED]> wrote:
> > > >
> > > > On Sep 26, 2006, at 9:39 AM, Evgueni Brevnov wrote:
> > > >
> > > > > On 9/26/06, Geir Magnusson Jr. <[EMAIL PROTECTED]> 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: [EMAIL PROTECTED]
> > > > >> For additional commands, e-mail: harmony-dev-
> > > > >> [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]
