Review request for "Enabling conditional inclusion of functional areas into the VM"

[David Holmes]

On 23/02/2012 12:59 PM, Tom Rodriguez wrote:
> On Feb 22, 2012, at 6:54 PM, David Holmes wrote:
>> On 23/02/2012 5:37 AM, Tom Rodriguez wrote:
>>> Instead of sprinkling #ifdef INCLUDE_JVMTI I think it would be cleaner to factor those chunks out as much as possible and ifdef them in JvmtiExport.  So something like this:
>>>
>>> + #ifdef INCLUDE_JVMTI
>>>      if (JvmtiExport::should_post_data_dump()) {
>>>        JvmtiExport::post_data_dump();
>>>      }
>>> + #endif // INCLUDE_JVMTI
>>>
>>> would be:
>>>
>>>    JvmtiExport::maybe_post_data_dump();
>>
>> The idea is to completely elide the jvmti*.cpp files from the build so there can be no JvmtiExport wrapper functions to hide the ifdef'd code.
>
> I guess I was confused by all the changes in jvmtiExport.hpp.  If the file isn't included in the build then why have any ifdefs in there at all?

Sorry my mistake. We can't completely elide all the jvmti* files as we 
need to be able to report that JVMTI is not supported.

> You could still do what I'm suggesting and just keep jvmtiExport.hpp around as the empty shell that produces no code.

Such a refactoring would look somewhat cleaner.

David

> tom
>
>>
>> David
>> -----
>>
>>> with:
>>>
>>> class JvmtiExport {
>>>    …
>>>    void maybe_post_data_dump() {
>>> #ifdef INCLUDE_JVMTI
>>>      if (should_post_data_dump()) {
>>>        post_data_dump();
>>>      }
>>> + #endif // INCLUDE_JVMTI
>>>    }
>>>
>>> or if should_post_data_dump always returned false and post_data_dump was empty then the outer ifdef wouldn't be needed all.  Either way would concentrate the ifdef's without cluttering up the rest of the source so much.  I think in cases where they are larger chunks inside the if, then that should be moved into JvmtiExport as well.  It won't work for everything but it will work for most things.
>>>
>>> In macros.hpp, most (all?) of the new _RETURN macros have their sense inverted from the norm.  PRODUCT_RETURN means ifdef PRODUCT then return.  The new ones do if !macro then return.  MINIMAL_JVM_RETURN has the proper sense.
>>>
>>> I would also like to avoid ifdefs in the middle of expressions.  Can't those be handled by NOT_JVMTI_RETURN_(false)?
>>>
>>> I think the changes in deoptimization.cpp don't look right.  I think can_pop_frame should return false if JVMTI is disabled.  The ifdefs act as if it returned true.
>>>
>>> The ifdef in instanceKlassKlass.cpp seems to cover too much code.
>>>
>>> Why are you keeping the ifdef KERNEL code in systemDictionary.cpp?  It can always be resurrected from the history if it's needed.
>>>
>>> I see changes in templateTable_x86_32.cpp but not in the other platform dependent templateTable code. Why is that?
>>>
>>> tom
>