• Type: Sub-task
• Component: javafx
• Sub-Component: web

• Priority: P3
• Status: Resolved
• Resolution: Fixed

• Submitted: 2014-10-17
• Updated: 2015-06-12
• Resolved: 2014-10-24

JDK 8
8u40Fixed

Fix pushed changeset: 8253:892b1bca2c5e user: loneid date: Fri Oct 24 18:01:15 2014 +0400 summary: Fixed RT-39013 Reduce memory footprint of WCImageDecoderImpl 24-10-2014 Looks fine to me. 24-10-2014 Thanks guys for your comments. Andrew: you're right, the so-called 'data adding protocol' is not formally defined, therefore I made the flag set to false when a not-null portion of data is added. I also renamed it to 'framesDecoded' for better clarity. Anton: I modified the comment to reflect the new method's semantics. Also, I made some minor changes like extracting repeating code blocks to a method or adding a few comments in the code. http://cr.openjdk.java.net/~loneid/RT-39013/v2/ 23-10-2014 Leonid, could you please clarify the following? Is that Ok that getFrameCount() returns non zero value after destroy() has been called? 20-10-2014 I think you can just modify the comments for the destroy() method. Also, does the frames==null && frameCount>=0 means the frames had been loaded but clear/destroy has been called afterwards? 20-10-2014 Is the "data adding protocol" formally defined anywhere? And are you completely sure that underlying data loading machinery is bug free? I agree that this should not happen under normal circumstances, but we have to minimize troubles if something went wrong with the data loading. The suggestion to skip an extra attempt to re-decode whole image sounds reasonable. 20-10-2014 Andrew: if we are in line 125 when dataLoadingComplete equals to true, we are in troubles: it means that the data adding protocol is corrupted. But your comment suggested me an idea that we probably don't need special processing for null data at all. Since big images are decoded incrementally as new portion of data is added, they will naturally get decoded completely via an asynchronous loader after the last data has been received, so decoding the whole image again when null data received looks redundant. Does it make sense? I will do some testing anyway. Anton: actually, the semantics of the destroy method now means something like 'destroyDecodedData'. Initially it was supposed to be called from the ImageSource destructor only, the proposed patch makes it called from ImageSource::clear() as well, what is reflected in the method's comment. Do you think I should have better introduced a separate method for this? 20-10-2014 Thanks for the clarification. Taking it into account, I would suggest to upgrade the condition at WCImageDecoderImpl.java, line 125 in order to prevent new loader creation if dataLoadingComplete is set. It protect us against a theoretical scenario where we create a new loader which we will be unable to stop and destroy. Wat do you think? 17-10-2014 Thanks Andrew. Explicit initialization made for 'dataLoadingComplete' and for 'frameCount' as well for more clarity: http://cr.openjdk.java.net/~loneid/RT-39013/v1/ The purpose of 'dataLoadingComplete' is to be set to 'true' only once, right after 'addImageData()' is called with null data for the first time, which means that raw data are completely loaded from a resource and will never change, while background loaders are started when only part of raw data gets loaded. Therefore, at the moment of starting a new loader, 'dataLoadingComplete' cannot be equal to 'true' so it doesn't need to be set to 'false'. Tracing calls to 'addImageData()' from Webkit shows that it is called with null data periodically regardless of future access to decoded frames, so 'dataLoadingComplete' guards frames from needless decoding. 17-10-2014 Memory footprint reduced by a) shrinking the data array to the actual data size after all the data received; b) making native ImageSource::clear() to call WCImageDecoderImpl.destroy() which clears decoded data, and then re-decoding them on demand. http://cr.openjdk.java.net/~loneid/RT-39013/v0 17-10-2014 Hello Leonid, Could you please explicitly initialize the new variable 'dataLoadingComplete'? And probably, this variable has to be re-set to false every time when we start new loader, hasn't it? Thanks, Andrew 17-10-2014