[OpenJDK 2D-Dev] [8] request for review: 8020983: OutOfMemoryError caused by non garbage collected JPEGImageWriter Instances

[Jim Graham]

I wasn't sure what the cinfo error exit did.  Is that like a 
setJmp/longJmp mechanism?

The new macro looks good to me from a maintainability point of view, but 
Phil is the expert on this code...

			...jim

On 7/29/13 11:53 PM, Andrew Brygin wrote:
> Hello,
>
>   I have updated the fix according to the comment:
>
>   * split the variable declaration and the macro
>
>   * re-wrote the macro in suggested manner
>
>   Please note that the macro forces an error exit if we
>   failed to get the io handler, so there seems to be no risk
>   of NPE.
>
>   Please take a look to updated fix:
>   http://cr.openjdk.java.net/~bae/8020983/8/webrev.02/
>
> Thanks,
> Andrew
>
> On 7/30/2013 4:18 AM, Jim Graham wrote:
>> One suggestion I'd add, though, is that the GET() macro is written so
>> as to be difficult to maintain.  It both declares a variable and
>> executes code which means it does not act like a declaration, nor does
>> it act like a piece of code.  Also, if someone needs to create a
>> similar type of macro later, they would have no ability to inject into
>> any code that uses this macro.
>>
>> I'd separate out the variable declaration and just have the macro deal
>> with loading the value.
>>
>> I also am not a fan of macros ending with an "if" statement since they
>> leave an optional state as their residue.  If the following statement
>> is an "else", the person using the macro may believe that the else
>> will apply to their "if", but it would actually apply to the "if" in
>> the macro:
>>
>>     if (mycondition)  MACRO(foo) else bar;
>>
>> In that case the "else bar;" would apply to the if inside the macro.
>> And, using it with a closing semicolon would yeild a compile error:
>>
>>     if (mycondition)  MACRO(foo); else bar;
>>
>> would generate an exception because the else comes after a null
>> statement due to the "};" that is expanded from the tail end of the
>> macro body.
>>
>> The style guide recommends always using braces because of this type of
>> case, but I prefer to write self-protecting macros:
>>
>> #define MACRO(foo) \
>>     do { \
>>            if (blah) stuff; \
>>         } while (0)
>>
>> The semicolon that typically follows the macro invocation would turn
>> this into a single statement compatible with just about any use in C
>> code.
>>
>> Additionally, I don't have my JNI handbook with me, but looking on
>> line I am not convinced that the follow-on JNI methods that try to
>> call a method on the (potentially null) stream can deal with a null
>> object to invoke on.  They don't list NPE as one of the exceptions
>> that they throw.  Some added logic to avoid invoking on a null stream
>> may be needed...
>>
>>             ...jim
>>
>> On 7/29/13 2:32 PM, Phil Race wrote:
>>> Looks fine.
>>>
>>> -phil.
>>>
>>> On 7/29/2013 2:13 PM, Andrew Brygin wrote:
>>>> Hello,
>>>>
>>>>  please take a look to updated fix:
>>>>
>>>>  http://cr.openjdk.java.net/~bae/8020983/8/webrev.01/
>>>>
>>>>  The name 'stream' is now eliminated.
>>>>
>>>>  I have verified that the change does not trigger any existing
>>>>  regression tests for imageio jpeg.
>>>>
>>>> Thanks,
>>>> Andrew
>>>>
>>>> On 7/29/2013 7:21 PM, Mario Torre wrote:
>>>>> On Mon, 2013-07-29 at 19:05 +0400, Andrew Brygin wrote:
>>>>>> Hi Mario,
>>>>>>
>>>>>>    thanks for the comments!
>>>>>>
>>>>>>    I agree that 'stream' and 'streamRef' are not appropriate names
>>>>>> now,
>>>>>>    because these entities point to instances of writer/reader.
>>>>>>
>>>>>>    However, these entities use the writer/reader only as a holder of
>>>>>>    certain set of I/O routines, so probably names for these entities
>>>>>>    should reflect their I/O nature.
>>>>>>
>>>>>>    Would 'ioRef' and 'input'/'output' sound better than 'stream'?
>>>>> Hi Andrew,
>>>>>
>>>>> Those seems better defaults, yes.
>>>>>
>>>>> At first I got confused with "stream" because the struct used to hold
>>>>> the Input/Output stream for real. I guess what really bothers me
>>>>> though
>>>>> is the comment:
>>>>>
>>>>> jweak streamRef; // ImageInputStream or ImageOutputStream
>>>>>                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>>>> Which is misleading now :) so thanks for addressing this!
>>>>>
>>>>> Cheers,
>>>>> Mario
>>>>>
>>>>>
>>>>
>>>
>