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

[Phil Race]

Fine by me.

-phil.

On 7/30/2013 3:53 PM, Jim Graham wrote:
> 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
>>>>>>
>>>>>>
>>>>>
>>>>
>>