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

[Andrew Brygin]

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

Thanks,
Andrew

On 7/29/2013 3:48 PM, Mario Torre wrote:
> On Mon, 2013-07-29 at 14:58 +0400, Andrew Brygin wrote:
>> Hello,
>>
>> could you please review a fix for CR 8020983?
>>
>> Bug: http://bugs.sun.com/view_bug.do?bug_id=8020983
>> Webrev: http://cr.openjdk.java.net/~bae/8020983/8/webrev.00/
>>
>> This fix replaces the strong reference in the streamBuffer structure
>> with a weak reference to prevent locking a reader/writer instance.
>>
>> A macro GET_STREAM_REF is introduced to obtain a reference to the
>> 'stream' with checks whether the reference is still alive. If not,
>> the plugin instance is terminated. However, this case seems to be
>> very unlikely because we need an alive plugin instance to run into
>> these methods.
>>
>> Supplied regression test verifies that writer instances can be
>> collected.
>>
>> Please take a look.
> Hi Andrew,
>
> I'm not a reviewer, but the fix looks good for me.
>
> One suggestion though, since streamBufferStruct->streamRef is a source
> of confusion.
>
> typedef struct streamBufferStruct {
>    jweak streamRef; // ImageInputStream or ImageOutputStream
>
> but unless I'm completely blind doesn't hold the input/output stream any
> longer. It actually contains JPEGImageReader/Writer references directly.
>
> It may be worth to update the comment as well while we are there to
> avoid confusion (this is why in our previous fix we didn't bother with
> the weak references, but yours is definitely a nicer fix).
>
> Cheers,
> Mario
>
>