You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@harmony.apache.org by gs...@apache.org on 2006/11/25 20:59:39 UTC
svn commit: r479181 -
/harmony/enhanced/drlvm/trunk/vm/vmcore/src/class_support/classloader.cpp
Author: gshimansky
Date: Sat Nov 25 11:59:38 2006
New Revision: 479181
URL: http://svn.apache.org/viewvc?view=rev&rev=479181
Log:
Fixed 32-bitness in classloader tracing. Added assertion before using a raw heap object pointer
Modified:
harmony/enhanced/drlvm/trunk/vm/vmcore/src/class_support/classloader.cpp
Modified: harmony/enhanced/drlvm/trunk/vm/vmcore/src/class_support/classloader.cpp
URL: http://svn.apache.org/viewvc/harmony/enhanced/drlvm/trunk/vm/vmcore/src/class_support/classloader.cpp?view=diff&rev=479181&r1=479180&r2=479181
==============================================================================
--- harmony/enhanced/drlvm/trunk/vm/vmcore/src/class_support/classloader.cpp (original)
+++ harmony/enhanced/drlvm/trunk/vm/vmcore/src/class_support/classloader.cpp Sat Nov 25 11:59:38 2006
@@ -589,11 +589,13 @@
ClassLoader* ClassLoader::AddClassLoader( ManagedObject* loader )
{
+ SuspendDisabledChecker sdc;
+
LMAutoUnlock aulock( &(ClassLoader::m_tableLock) );
ClassLoader* cl = new UserDefinedClassLoader();
TRACE2("classloader.unloading.add", "Adding class loader "
<< cl << " (" << loader << " : "
- << ((VTable*)(*(unsigned**)(loader)))->clss->get_name()->bytes << ")");
+ << loader->vt()->clss->get_name()->bytes << ")");
cl->Initialize( loader );
if( m_capacity <= m_nextEntry )
ReallocateTable( m_capacity?(2*m_capacity):32 );
Re: svn commit: r479181 - /harmony/enhanced/drlvm/trunk/vm/vmcore/src/class_support/classloader.cpp
Posted by Evgueni Brevnov <ev...@gmail.com>.
I just noted this discussion. I support an idea of checking thread's
suspend enabled/disabled state automatically. It seems to be useful
and convenient approach to me. To be honest I don't see huge
difference between two names SuspendDisabledChecker &
EnableDisableAutoAssert. Though the last one sounds better.
Thanks
Evgueni
On 11/28/06, Gregory Shimansky <gs...@gmail.com> wrote:
> Geir Magnusson Jr. wrote:
> >
> >
> > Gregory Shimansky wrote:
> >> Geir Magnusson Jr. wrote:
> >>>
> >>>
> >>> Gregory Shimansky wrote:
> >>>> Geir Magnusson Jr. wrote:
> >>>>>
> >>>>>
> >>>>> Gregory Shimansky wrote:
> >>>
> >>>>>>
> >>>>>> In VM code SuspendChecker usage is clear enough, it is quite
> >>>>>> widely used, although there is a lot of code which wasn't
> >>>>>> converted since checker classes were introduced. New code uses
> >>>>>> them, old code uses assertions.
> >>>>>
> >>>>> It's not clear to a casual reader. I don't know if a rename is in
> >>>>> order, or the addition of a comment when used, but clearly to claim
> >>>>> that you added an "assert" and then use a class called "Checker" is
> >>>>> far from transparent.
> >>>>>
> >>>>> geir
> >>>>
> >>>> I'll try to make more descriptive commit comments :)
> >>>
> >>> No - you're missing my point completely. it isn't about the commit
> >>> comments, but rather the source. Because of the unorthodox name of
> >>> the class and lack of any comment *in the code* it's unclear to a
> >>> reader what's going on.
> >>>
> >>> It was certainly unclear to me.
> >>>
> >>> The class is a cute trick, and I can see how it makes life easier for
> >>> maintenance, but we also have to consider that this code is being
> >>> written for collective ownership and maintenance, so the more
> >>> explicit and clear we can be, the better off the project is.
> >>>
> >>> Always write code assuming that you win the lottery and retire
> >>> tomorrow, so make it clear enough that there are few questions.
> >>
> >> If you grep the source you'll find that Suspend.*Checker classes are
> >> used a lot. Do you suggest to put a comment before every use? It is
> >> just the first time you've seen this call, so it was not clear to you.
> >> Simply finding the class definition would've answered all your
> >> questions. It is a normal thing to do when you see that some function
> >> is called, and don't understand what it does.
> >
> > Why use names at all then? Why not "A", "B", "C" for your
> > classnames...? (and "a", "b" "c" for your function names...)
> >
> > Then if someone points out that it made the code not obvious to the
> > reader, we can just tell them to use grep and look it up....
> >
> > :D
>
> I didn't think that the name is so unclear, and the main problem was
> that it is unclear why define a local variable which is not used.
>
> >>
> >> I understand the need to document the code and put comments into it. I
> >> also think that vmcore/include/suspend_checker.h could have some
> >> doxyden docs. But putting identical comments like "check the that
> >> suspend is enabled/disabled when entering and leaving this function"
> >> all around the code, seems to be too excessive to me.
> >>
> >
> > How about renaming it? "EnableDisableAutoAssert" or something?
>
> Ok I'll put it on my todo list.
>
> --
> Gregory
>
>
Re: svn commit: r479181 - /harmony/enhanced/drlvm/trunk/vm/vmcore/src/class_support/classloader.cpp
Posted by Gregory Shimansky <gs...@gmail.com>.
Geir Magnusson Jr. wrote:
>
>
> Gregory Shimansky wrote:
>> Geir Magnusson Jr. wrote:
>>>
>>>
>>> Gregory Shimansky wrote:
>>>> Geir Magnusson Jr. wrote:
>>>>>
>>>>>
>>>>> Gregory Shimansky wrote:
>>>
>>>>>>
>>>>>> In VM code SuspendChecker usage is clear enough, it is quite
>>>>>> widely used, although there is a lot of code which wasn't
>>>>>> converted since checker classes were introduced. New code uses
>>>>>> them, old code uses assertions.
>>>>>
>>>>> It's not clear to a casual reader. I don't know if a rename is in
>>>>> order, or the addition of a comment when used, but clearly to claim
>>>>> that you added an "assert" and then use a class called "Checker" is
>>>>> far from transparent.
>>>>>
>>>>> geir
>>>>
>>>> I'll try to make more descriptive commit comments :)
>>>
>>> No - you're missing my point completely. it isn't about the commit
>>> comments, but rather the source. Because of the unorthodox name of
>>> the class and lack of any comment *in the code* it's unclear to a
>>> reader what's going on.
>>>
>>> It was certainly unclear to me.
>>>
>>> The class is a cute trick, and I can see how it makes life easier for
>>> maintenance, but we also have to consider that this code is being
>>> written for collective ownership and maintenance, so the more
>>> explicit and clear we can be, the better off the project is.
>>>
>>> Always write code assuming that you win the lottery and retire
>>> tomorrow, so make it clear enough that there are few questions.
>>
>> If you grep the source you'll find that Suspend.*Checker classes are
>> used a lot. Do you suggest to put a comment before every use? It is
>> just the first time you've seen this call, so it was not clear to you.
>> Simply finding the class definition would've answered all your
>> questions. It is a normal thing to do when you see that some function
>> is called, and don't understand what it does.
>
> Why use names at all then? Why not "A", "B", "C" for your
> classnames...? (and "a", "b" "c" for your function names...)
>
> Then if someone points out that it made the code not obvious to the
> reader, we can just tell them to use grep and look it up....
>
> :D
I didn't think that the name is so unclear, and the main problem was
that it is unclear why define a local variable which is not used.
>>
>> I understand the need to document the code and put comments into it. I
>> also think that vmcore/include/suspend_checker.h could have some
>> doxyden docs. But putting identical comments like "check the that
>> suspend is enabled/disabled when entering and leaving this function"
>> all around the code, seems to be too excessive to me.
>>
>
> How about renaming it? "EnableDisableAutoAssert" or something?
Ok I'll put it on my todo list.
--
Gregory
Re: svn commit: r479181 - /harmony/enhanced/drlvm/trunk/vm/vmcore/src/class_support/classloader.cpp
Posted by "Geir Magnusson Jr." <ge...@pobox.com>.
Gregory Shimansky wrote:
> Geir Magnusson Jr. wrote:
>>
>>
>> Gregory Shimansky wrote:
>>> Geir Magnusson Jr. wrote:
>>>>
>>>>
>>>> Gregory Shimansky wrote:
>>
>>>>>
>>>>> In VM code SuspendChecker usage is clear enough, it is quite widely
>>>>> used, although there is a lot of code which wasn't converted since
>>>>> checker classes were introduced. New code uses them, old code uses
>>>>> assertions.
>>>>
>>>> It's not clear to a casual reader. I don't know if a rename is in
>>>> order, or the addition of a comment when used, but clearly to claim
>>>> that you added an "assert" and then use a class called "Checker" is
>>>> far from transparent.
>>>>
>>>> geir
>>>
>>> I'll try to make more descriptive commit comments :)
>>
>> No - you're missing my point completely. it isn't about the commit
>> comments, but rather the source. Because of the unorthodox name of
>> the class and lack of any comment *in the code* it's unclear to a
>> reader what's going on.
>>
>> It was certainly unclear to me.
>>
>> The class is a cute trick, and I can see how it makes life easier for
>> maintenance, but we also have to consider that this code is being
>> written for collective ownership and maintenance, so the more explicit
>> and clear we can be, the better off the project is.
>>
>> Always write code assuming that you win the lottery and retire
>> tomorrow, so make it clear enough that there are few questions.
>
> If you grep the source you'll find that Suspend.*Checker classes are
> used a lot. Do you suggest to put a comment before every use? It is just
> the first time you've seen this call, so it was not clear to you. Simply
> finding the class definition would've answered all your questions. It is
> a normal thing to do when you see that some function is called, and
> don't understand what it does.
Why use names at all then? Why not "A", "B", "C" for your
classnames...? (and "a", "b" "c" for your function names...)
Then if someone points out that it made the code not obvious to the
reader, we can just tell them to use grep and look it up....
:D
>
> I understand the need to document the code and put comments into it. I
> also think that vmcore/include/suspend_checker.h could have some doxyden
> docs. But putting identical comments like "check the that suspend is
> enabled/disabled when entering and leaving this function" all around the
> code, seems to be too excessive to me.
>
How about renaming it? "EnableDisableAutoAssert" or something?
geir
Re: svn commit: r479181 - /harmony/enhanced/drlvm/trunk/vm/vmcore/src/class_support/classloader.cpp
Posted by Gregory Shimansky <gs...@gmail.com>.
Geir Magnusson Jr. wrote:
>
>
> Gregory Shimansky wrote:
>> Geir Magnusson Jr. wrote:
>>>
>>>
>>> Gregory Shimansky wrote:
>
>>>>
>>>> In VM code SuspendChecker usage is clear enough, it is quite widely
>>>> used, although there is a lot of code which wasn't converted since
>>>> checker classes were introduced. New code uses them, old code uses
>>>> assertions.
>>>
>>> It's not clear to a casual reader. I don't know if a rename is in
>>> order, or the addition of a comment when used, but clearly to claim
>>> that you added an "assert" and then use a class called "Checker" is
>>> far from transparent.
>>>
>>> geir
>>
>> I'll try to make more descriptive commit comments :)
>
> No - you're missing my point completely. it isn't about the commit
> comments, but rather the source. Because of the unorthodox name of the
> class and lack of any comment *in the code* it's unclear to a reader
> what's going on.
>
> It was certainly unclear to me.
>
> The class is a cute trick, and I can see how it makes life easier for
> maintenance, but we also have to consider that this code is being
> written for collective ownership and maintenance, so the more explicit
> and clear we can be, the better off the project is.
>
> Always write code assuming that you win the lottery and retire tomorrow,
> so make it clear enough that there are few questions.
If you grep the source you'll find that Suspend.*Checker classes are
used a lot. Do you suggest to put a comment before every use? It is just
the first time you've seen this call, so it was not clear to you. Simply
finding the class definition would've answered all your questions. It is
a normal thing to do when you see that some function is called, and
don't understand what it does.
I understand the need to document the code and put comments into it. I
also think that vmcore/include/suspend_checker.h could have some doxyden
docs. But putting identical comments like "check the that suspend is
enabled/disabled when entering and leaving this function" all around the
code, seems to be too excessive to me.
--
Gregory
Re: svn commit: r479181 - /harmony/enhanced/drlvm/trunk/vm/vmcore/src/class_support/classloader.cpp
Posted by "Geir Magnusson Jr." <ge...@pobox.com>.
Gregory Shimansky wrote:
> Geir Magnusson Jr. wrote:
>>
>>
>> Gregory Shimansky wrote:
>>>
>>> In VM code SuspendChecker usage is clear enough, it is quite widely
>>> used, although there is a lot of code which wasn't converted since
>>> checker classes were introduced. New code uses them, old code uses
>>> assertions.
>>
>> It's not clear to a casual reader. I don't know if a rename is in
>> order, or the addition of a comment when used, but clearly to claim
>> that you added an "assert" and then use a class called "Checker" is
>> far from transparent.
>>
>> geir
>
> I'll try to make more descriptive commit comments :)
No - you're missing my point completely. it isn't about the commit
comments, but rather the source. Because of the unorthodox name of the
class and lack of any comment *in the code* it's unclear to a reader
what's going on.
It was certainly unclear to me.
The class is a cute trick, and I can see how it makes life easier for
maintenance, but we also have to consider that this code is being
written for collective ownership and maintenance, so the more explicit
and clear we can be, the better off the project is.
Always write code assuming that you win the lottery and retire tomorrow,
so make it clear enough that there are few questions.
geir
Re: svn commit: r479181 - /harmony/enhanced/drlvm/trunk/vm/vmcore/src/class_support/classloader.cpp
Posted by Gregory Shimansky <gs...@gmail.com>.
Geir Magnusson Jr. wrote:
>
>
> Gregory Shimansky wrote:
>> Geir Magnusson Jr. wrote:
>>>
>>>
>>> Gregory Shimansky wrote:
>>>> On Sunday 26 November 2006 03:30 Geir Magnusson Jr. wrote:
>>>>> 1) Why add the SuspendDisabledChecker if not using it?
>>>>>
>>>>> 2) exactly where did you add the assertion? :)
>>>>
>>>> It is a hidden assertion class :)
>>>
>>> Oh, come on. An assertion as a side effect?
>>
>> No, assertion is one and the only purpose of this class. Isn't word
>> Checker in class name is not descriptive enough.
>
> No, clearly not :)
>
>>
>>>>
>>>> Look at the file vm/vmcore/include/suspend_checker.h. There are 2
>>>> classes SuspendEnabledChecker and SuspendDisabledChecker. When
>>>> declared, such variable in constructor checks for suspend status, in
>>>> destructor it checks that the status is the same. It is often
>>>> convenient to write just this one line to make sure that all returns
>>>> from the function have the same suspend status because local
>>>> variable destructor is executed on every function exit.
>>>>
>>>> In release, when assert is a noop, these constructors/destructors
>>>> are optimized into noop as well.
>>>>
>>>> I saw that this function uses raw ManagedObject pointers. This is
>>>> dangerous in case when suspend is enabled (equal to GC being
>>>> enabled) as the object may be moved at any time. So I decided to add
>>>> this assertion. If it fails some time it will signal that this
>>>> function is called in a wrong unsafe mode.
>>>
>>> Ok - but don't you think that the assert() would be clearer and just
>>> as efficient?
>>
>> In this function maybe, but in other places where it is used, the fact
>> that all return sites in the function are covered by this one line is
>> very convenient. Also if function changes, someone may forget to put
>> assertion before some return statement.
>
> I'll admit, it is darn convenient.
>
>>
>> In VM code SuspendChecker usage is clear enough, it is quite widely
>> used, although there is a lot of code which wasn't converted since
>> checker classes were introduced. New code uses them, old code uses
>> assertions.
>
> It's not clear to a casual reader. I don't know if a rename is in
> order, or the addition of a comment when used, but clearly to claim that
> you added an "assert" and then use a class called "Checker" is far from
> transparent.
>
> geir
I'll try to make more descriptive commit comments :)
--
Gregory
Re: svn commit: r479181 - /harmony/enhanced/drlvm/trunk/vm/vmcore/src/class_support/classloader.cpp
Posted by "Geir Magnusson Jr." <ge...@pobox.com>.
Gregory Shimansky wrote:
> Geir Magnusson Jr. wrote:
>>
>>
>> Gregory Shimansky wrote:
>>> On Sunday 26 November 2006 03:30 Geir Magnusson Jr. wrote:
>>>> 1) Why add the SuspendDisabledChecker if not using it?
>>>>
>>>> 2) exactly where did you add the assertion? :)
>>>
>>> It is a hidden assertion class :)
>>
>> Oh, come on. An assertion as a side effect?
>
> No, assertion is one and the only purpose of this class. Isn't word
> Checker in class name is not descriptive enough.
No, clearly not :)
>
>>>
>>> Look at the file vm/vmcore/include/suspend_checker.h. There are 2
>>> classes SuspendEnabledChecker and SuspendDisabledChecker. When
>>> declared, such variable in constructor checks for suspend status, in
>>> destructor it checks that the status is the same. It is often
>>> convenient to write just this one line to make sure that all returns
>>> from the function have the same suspend status because local variable
>>> destructor is executed on every function exit.
>>>
>>> In release, when assert is a noop, these constructors/destructors are
>>> optimized into noop as well.
>>>
>>> I saw that this function uses raw ManagedObject pointers. This is
>>> dangerous in case when suspend is enabled (equal to GC being enabled)
>>> as the object may be moved at any time. So I decided to add this
>>> assertion. If it fails some time it will signal that this function is
>>> called in a wrong unsafe mode.
>>
>> Ok - but don't you think that the assert() would be clearer and just
>> as efficient?
>
> In this function maybe, but in other places where it is used, the fact
> that all return sites in the function are covered by this one line is
> very convenient. Also if function changes, someone may forget to put
> assertion before some return statement.
I'll admit, it is darn convenient.
>
> In VM code SuspendChecker usage is clear enough, it is quite widely
> used, although there is a lot of code which wasn't converted since
> checker classes were introduced. New code uses them, old code uses
> assertions.
It's not clear to a casual reader. I don't know if a rename is in
order, or the addition of a comment when used, but clearly to claim that
you added an "assert" and then use a class called "Checker" is far from
transparent.
geir
>
>>>
>>>> gshimansky@apache.org wrote:
>>>>> Author: gshimansky
>>>>> Date: Sat Nov 25 11:59:38 2006
>>>>> New Revision: 479181
>>>>>
>>>>> URL: http://svn.apache.org/viewvc?view=rev&rev=479181
>>>>> Log:
>>>>> Fixed 32-bitness in classloader tracing. Added assertion before
>>>>> using a
>>>>> raw heap object pointer
>>>>>
>>>>>
>>>>> Modified:
>>>>>
>>>>> harmony/enhanced/drlvm/trunk/vm/vmcore/src/class_support/classloader.cpp
>>>>>
>>>>>
>>>>> Modified:
>>>>> harmony/enhanced/drlvm/trunk/vm/vmcore/src/class_support/classloader.cpp
>>>>>
>>>>> URL:
>>>>> http://svn.apache.org/viewvc/harmony/enhanced/drlvm/trunk/vm/vmcore/src/c
>>>>>
>>>>> lass_support/classloader.cpp?view=diff&rev=479181&r1=479180&r2=479181
>>>>> =========================================================================
>>>>>
>>>>> ===== ---
>>>>> harmony/enhanced/drlvm/trunk/vm/vmcore/src/class_support/classloader.cpp
>>>>>
>>>>> (original) +++
>>>>> harmony/enhanced/drlvm/trunk/vm/vmcore/src/class_support/classloader.cpp
>>>>>
>>>>> Sat Nov 25 11:59:38 2006 @@ -589,11 +589,13 @@
>>>>>
>>>>> ClassLoader* ClassLoader::AddClassLoader( ManagedObject* loader )
>>>>> {
>>>>> + SuspendDisabledChecker sdc;
>>>>> +
>>>>> LMAutoUnlock aulock( &(ClassLoader::m_tableLock) );
>>>>> ClassLoader* cl = new UserDefinedClassLoader();
>>>>> TRACE2("classloader.unloading.add", "Adding class loader "
>>>>> << cl << " (" << loader << " : "
>>>>> - <<
>>>>> ((VTable*)(*(unsigned**)(loader)))->clss->get_name()->bytes
>>>>> << ")"); + << loader->vt()->clss->get_name()->bytes << ")");
>>>>> cl->Initialize( loader );
>>>>> if( m_capacity <= m_nextEntry )
>>>>> ReallocateTable( m_capacity?(2*m_capacity):32 );
>>>
>>
>
>
Re: svn commit: r479181 - /harmony/enhanced/drlvm/trunk/vm/vmcore/src/class_support/classloader.cpp
Posted by Gregory Shimansky <gs...@gmail.com>.
Geir Magnusson Jr. wrote:
>
>
> Gregory Shimansky wrote:
>> On Sunday 26 November 2006 03:30 Geir Magnusson Jr. wrote:
>>> 1) Why add the SuspendDisabledChecker if not using it?
>>>
>>> 2) exactly where did you add the assertion? :)
>>
>> It is a hidden assertion class :)
>
> Oh, come on. An assertion as a side effect?
No, assertion is one and the only purpose of this class. Isn't word
Checker in class name is not descriptive enough.
>>
>> Look at the file vm/vmcore/include/suspend_checker.h. There are 2
>> classes SuspendEnabledChecker and SuspendDisabledChecker. When
>> declared, such variable in constructor checks for suspend status, in
>> destructor it checks that the status is the same. It is often
>> convenient to write just this one line to make sure that all returns
>> from the function have the same suspend status because local variable
>> destructor is executed on every function exit.
>>
>> In release, when assert is a noop, these constructors/destructors are
>> optimized into noop as well.
>>
>> I saw that this function uses raw ManagedObject pointers. This is
>> dangerous in case when suspend is enabled (equal to GC being enabled)
>> as the object may be moved at any time. So I decided to add this
>> assertion. If it fails some time it will signal that this function is
>> called in a wrong unsafe mode.
>
> Ok - but don't you think that the assert() would be clearer and just as
> efficient?
In this function maybe, but in other places where it is used, the fact
that all return sites in the function are covered by this one line is
very convenient. Also if function changes, someone may forget to put
assertion before some return statement.
In VM code SuspendChecker usage is clear enough, it is quite widely
used, although there is a lot of code which wasn't converted since
checker classes were introduced. New code uses them, old code uses
assertions.
>>
>>> gshimansky@apache.org wrote:
>>>> Author: gshimansky
>>>> Date: Sat Nov 25 11:59:38 2006
>>>> New Revision: 479181
>>>>
>>>> URL: http://svn.apache.org/viewvc?view=rev&rev=479181
>>>> Log:
>>>> Fixed 32-bitness in classloader tracing. Added assertion before using a
>>>> raw heap object pointer
>>>>
>>>>
>>>> Modified:
>>>>
>>>> harmony/enhanced/drlvm/trunk/vm/vmcore/src/class_support/classloader.cpp
>>>>
>>>>
>>>> Modified:
>>>> harmony/enhanced/drlvm/trunk/vm/vmcore/src/class_support/classloader.cpp
>>>>
>>>> URL:
>>>> http://svn.apache.org/viewvc/harmony/enhanced/drlvm/trunk/vm/vmcore/src/c
>>>>
>>>> lass_support/classloader.cpp?view=diff&rev=479181&r1=479180&r2=479181
>>>> =========================================================================
>>>>
>>>> ===== ---
>>>> harmony/enhanced/drlvm/trunk/vm/vmcore/src/class_support/classloader.cpp
>>>>
>>>> (original) +++
>>>> harmony/enhanced/drlvm/trunk/vm/vmcore/src/class_support/classloader.cpp
>>>>
>>>> Sat Nov 25 11:59:38 2006 @@ -589,11 +589,13 @@
>>>>
>>>> ClassLoader* ClassLoader::AddClassLoader( ManagedObject* loader )
>>>> {
>>>> + SuspendDisabledChecker sdc;
>>>> +
>>>> LMAutoUnlock aulock( &(ClassLoader::m_tableLock) );
>>>> ClassLoader* cl = new UserDefinedClassLoader();
>>>> TRACE2("classloader.unloading.add", "Adding class loader "
>>>> << cl << " (" << loader << " : "
>>>> - << ((VTable*)(*(unsigned**)(loader)))->clss->get_name()->bytes
>>>> << ")"); + << loader->vt()->clss->get_name()->bytes << ")");
>>>> cl->Initialize( loader );
>>>> if( m_capacity <= m_nextEntry )
>>>> ReallocateTable( m_capacity?(2*m_capacity):32 );
>>
>
--
Gregory
Re: svn commit: r479181 - /harmony/enhanced/drlvm/trunk/vm/vmcore/src/class_support/classloader.cpp
Posted by "Geir Magnusson Jr." <ge...@pobox.com>.
Gregory Shimansky wrote:
> On Sunday 26 November 2006 03:30 Geir Magnusson Jr. wrote:
>> 1) Why add the SuspendDisabledChecker if not using it?
>>
>> 2) exactly where did you add the assertion? :)
>
> It is a hidden assertion class :)
Oh, come on. An assertion as a side effect?
>
> Look at the file vm/vmcore/include/suspend_checker.h. There are 2 classes
> SuspendEnabledChecker and SuspendDisabledChecker. When declared, such
> variable in constructor checks for suspend status, in destructor it checks
> that the status is the same. It is often convenient to write just this one
> line to make sure that all returns from the function have the same suspend
> status because local variable destructor is executed on every function exit.
>
> In release, when assert is a noop, these constructors/destructors are
> optimized into noop as well.
>
> I saw that this function uses raw ManagedObject pointers. This is dangerous in
> case when suspend is enabled (equal to GC being enabled) as the object may be
> moved at any time. So I decided to add this assertion. If it fails some time
> it will signal that this function is called in a wrong unsafe mode.
Ok - but don't you think that the assert() would be clearer and just as
efficient?
geir
>
>> gshimansky@apache.org wrote:
>>> Author: gshimansky
>>> Date: Sat Nov 25 11:59:38 2006
>>> New Revision: 479181
>>>
>>> URL: http://svn.apache.org/viewvc?view=rev&rev=479181
>>> Log:
>>> Fixed 32-bitness in classloader tracing. Added assertion before using a
>>> raw heap object pointer
>>>
>>>
>>> Modified:
>>>
>>> harmony/enhanced/drlvm/trunk/vm/vmcore/src/class_support/classloader.cpp
>>>
>>> Modified:
>>> harmony/enhanced/drlvm/trunk/vm/vmcore/src/class_support/classloader.cpp
>>> URL:
>>> http://svn.apache.org/viewvc/harmony/enhanced/drlvm/trunk/vm/vmcore/src/c
>>> lass_support/classloader.cpp?view=diff&rev=479181&r1=479180&r2=479181
>>> =========================================================================
>>> ===== ---
>>> harmony/enhanced/drlvm/trunk/vm/vmcore/src/class_support/classloader.cpp
>>> (original) +++
>>> harmony/enhanced/drlvm/trunk/vm/vmcore/src/class_support/classloader.cpp
>>> Sat Nov 25 11:59:38 2006 @@ -589,11 +589,13 @@
>>>
>>> ClassLoader* ClassLoader::AddClassLoader( ManagedObject* loader )
>>> {
>>> + SuspendDisabledChecker sdc;
>>> +
>>> LMAutoUnlock aulock( &(ClassLoader::m_tableLock) );
>>> ClassLoader* cl = new UserDefinedClassLoader();
>>> TRACE2("classloader.unloading.add", "Adding class loader "
>>> << cl << " (" << loader << " : "
>>> - << ((VTable*)(*(unsigned**)(loader)))->clss->get_name()->bytes
>>> << ")"); + << loader->vt()->clss->get_name()->bytes << ")");
>>> cl->Initialize( loader );
>>> if( m_capacity <= m_nextEntry )
>>> ReallocateTable( m_capacity?(2*m_capacity):32 );
>
Re: svn commit: r479181 - /harmony/enhanced/drlvm/trunk/vm/vmcore/src/class_support/classloader.cpp
Posted by Gregory Shimansky <gs...@gmail.com>.
On Sunday 26 November 2006 03:30 Geir Magnusson Jr. wrote:
> 1) Why add the SuspendDisabledChecker if not using it?
>
> 2) exactly where did you add the assertion? :)
It is a hidden assertion class :)
Look at the file vm/vmcore/include/suspend_checker.h. There are 2 classes
SuspendEnabledChecker and SuspendDisabledChecker. When declared, such
variable in constructor checks for suspend status, in destructor it checks
that the status is the same. It is often convenient to write just this one
line to make sure that all returns from the function have the same suspend
status because local variable destructor is executed on every function exit.
In release, when assert is a noop, these constructors/destructors are
optimized into noop as well.
I saw that this function uses raw ManagedObject pointers. This is dangerous in
case when suspend is enabled (equal to GC being enabled) as the object may be
moved at any time. So I decided to add this assertion. If it fails some time
it will signal that this function is called in a wrong unsafe mode.
> gshimansky@apache.org wrote:
> > Author: gshimansky
> > Date: Sat Nov 25 11:59:38 2006
> > New Revision: 479181
> >
> > URL: http://svn.apache.org/viewvc?view=rev&rev=479181
> > Log:
> > Fixed 32-bitness in classloader tracing. Added assertion before using a
> > raw heap object pointer
> >
> >
> > Modified:
> >
> > harmony/enhanced/drlvm/trunk/vm/vmcore/src/class_support/classloader.cpp
> >
> > Modified:
> > harmony/enhanced/drlvm/trunk/vm/vmcore/src/class_support/classloader.cpp
> > URL:
> > http://svn.apache.org/viewvc/harmony/enhanced/drlvm/trunk/vm/vmcore/src/c
> >lass_support/classloader.cpp?view=diff&rev=479181&r1=479180&r2=479181
> > =========================================================================
> >===== ---
> > harmony/enhanced/drlvm/trunk/vm/vmcore/src/class_support/classloader.cpp
> > (original) +++
> > harmony/enhanced/drlvm/trunk/vm/vmcore/src/class_support/classloader.cpp
> > Sat Nov 25 11:59:38 2006 @@ -589,11 +589,13 @@
> >
> > ClassLoader* ClassLoader::AddClassLoader( ManagedObject* loader )
> > {
> > + SuspendDisabledChecker sdc;
> > +
> > LMAutoUnlock aulock( &(ClassLoader::m_tableLock) );
> > ClassLoader* cl = new UserDefinedClassLoader();
> > TRACE2("classloader.unloading.add", "Adding class loader "
> > << cl << " (" << loader << " : "
> > - << ((VTable*)(*(unsigned**)(loader)))->clss->get_name()->bytes
> > << ")"); + << loader->vt()->clss->get_name()->bytes << ")");
> > cl->Initialize( loader );
> > if( m_capacity <= m_nextEntry )
> > ReallocateTable( m_capacity?(2*m_capacity):32 );
--
Gregory
Re: svn commit: r479181 - /harmony/enhanced/drlvm/trunk/vm/vmcore/src/class_support/classloader.cpp
Posted by "Geir Magnusson Jr." <ge...@pobox.com>.
1) Why add the SuspendDisabledChecker if not using it?
2) exactly where did you add the assertion? :)
gshimansky@apache.org wrote:
> Author: gshimansky
> Date: Sat Nov 25 11:59:38 2006
> New Revision: 479181
>
> URL: http://svn.apache.org/viewvc?view=rev&rev=479181
> Log:
> Fixed 32-bitness in classloader tracing. Added assertion before using a raw heap object pointer
>
>
> Modified:
> harmony/enhanced/drlvm/trunk/vm/vmcore/src/class_support/classloader.cpp
>
> Modified: harmony/enhanced/drlvm/trunk/vm/vmcore/src/class_support/classloader.cpp
> URL: http://svn.apache.org/viewvc/harmony/enhanced/drlvm/trunk/vm/vmcore/src/class_support/classloader.cpp?view=diff&rev=479181&r1=479180&r2=479181
> ==============================================================================
> --- harmony/enhanced/drlvm/trunk/vm/vmcore/src/class_support/classloader.cpp (original)
> +++ harmony/enhanced/drlvm/trunk/vm/vmcore/src/class_support/classloader.cpp Sat Nov 25 11:59:38 2006
> @@ -589,11 +589,13 @@
>
> ClassLoader* ClassLoader::AddClassLoader( ManagedObject* loader )
> {
> + SuspendDisabledChecker sdc;
> +
> LMAutoUnlock aulock( &(ClassLoader::m_tableLock) );
> ClassLoader* cl = new UserDefinedClassLoader();
> TRACE2("classloader.unloading.add", "Adding class loader "
> << cl << " (" << loader << " : "
> - << ((VTable*)(*(unsigned**)(loader)))->clss->get_name()->bytes << ")");
> + << loader->vt()->clss->get_name()->bytes << ")");
> cl->Initialize( loader );
> if( m_capacity <= m_nextEntry )
> ReallocateTable( m_capacity?(2*m_capacity):32 );
>
>