harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gregory Shimansky <gshiman...@gmail.com>
Subject Re: svn commit: r479181 - /harmony/enhanced/drlvm/trunk/vm/vmcore/src/class_support/classloader.cpp
Date Mon, 27 Nov 2006 15:27:50 GMT
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


Mime
View raw message