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

Artem Ananiev artem.ananiev at oracle.com
Tue Jan 31 03:10:31 PST 2012


Looks fine.

Thanks,

Artem

On 1/31/2012 3:36 PM, Dmitry Cherepanov wrote:
> 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
>


More information about the macosx-port-dev mailing list