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

[Andrew Brygin]

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
>>>>
>>>>
>>>
>>