Review request for 7131793: [macosx] some cleanup in OGL pipeline code

[Dmitry Cherepanov]

Mike Swingler wrote:
> On Jan 30, 2012, at 5:59 AM, Dmitry Cherepanov wrote:
>
>   
>> Mike Swingler wrote:
>>
>>     
>>> On Jan 27, 2012, at 7:17 AM, Dmitry Cherepanov wrote:
>>>
>>>       
>>>> Hello,
>>>>
>>>> Here's some changes to remove some stale code from the OGL pipeline. This code has been introduced as a part of the initial implementation for CALayer-based rendering and disabled by the changes for MACOSX_PORT-766.
>>>>
>>>> http://cr.openjdk.java.net/~dcherepanov/7131793/webrev.1/ <http://cr.openjdk.java.net/%7Edcherepanov/7131793/webrev.1/>
>>>>         
>>> One tiny detail: I'd recommend using -jObjectWithEnv: instead of -jObject here (because it's a little faster to not re-fetch the env):
>>> +    JNFCallVoidMethod(env, [self.javaLayer jObject], jm_drawInCGLContext);
>>>       
>> On second thought, seems like -jObject simply returns the global reference that the wrapper holds without obtaining the env. What's the reason for re-fetching the env?
>>     
>
> That is correct...we are returning the global env, I had forgot about that. It doesn't seem safe though, if the wrapper gets dealloc'd while the ref is still held in a local. :-/
>
> As is though, I don't think we can change the API at this point.
>   
Thanks for the clarification.

Mike Swingler wrote:
> On Jan 30, 2012, at 1:52 AM, Sergey Bylokhov wrote:
>   
>>> 124JNFCallVoidMethod(env, [self.javaLayer jObjectWithEnv:env], jm_drawInCGLContext);
>>>
>>> Should we delete a local reference which returned from [self.javaLayer jObjectWithEnv:env]? And check it for null?
>>>       
>> JNFJObjectWrapper.h
>> ......
>> - (jobject) jObjectWithEnv:(JNIEnv *)env; // returns a new local-ref, must be released with DeleteLocalRef
>> ......
>>     
> I believe local refs are collected after the scope of the local function returns, so I don't believe it's strictly necessary.
>
> Is that right?
>   
Seems like it's better to explicitly free local references in native 
callbacks. The doc in 
http://java.sun.com/docs/books/jni/html/refs.html#27567 ("Freeing Local 
References" section) describes:

"Your native method does not return at all. For example, a native method 
may enter an infinite event dispatch loop. It is crucial to release 
local references created inside the loop so that they do not accumulate 
indefinitely, resulting in a memory leak."

Here's the new webrev (with explicit releasing applied) - 
http://cr.openjdk.java.net/~dcherepanov/7131793/webrev.3/

Thanks,
Dmitry