search

Reading-eScience-Centre / edal-java

Environmental Data Abstraction Layer libraries
Other
39 stars 30 forks source link

synchronized writeImage in PngFormat #97

Closed ghansham closed 6 years ago

ghansham commented 6 years ago

https://github.com/Reading-eScience-Centre/edal-java/blob/ed5a7803136037a38d05c2f791e3415e62702bf0/graphics/src/main/java/uk/ac/rdg/resc/edal/graphics/formats/PngFormat.java#L72

Why this method is synchronized while all the other classes (JpegFormat, GifFormat, etc) that are derived from SimpleFormat dont have this method as synchronized? This may be a performance bottleneck. Another thing I noticed is that for some lookup table (e.g. seq-Heat lookup table) generated Images take longer time in PngFormat writeImage method.

guygriffiths commented 6 years ago

I'm not sure. @jonblower - can you recall why it was done this way? I doubt it's likely to cause any bottlenecks - I/O is by far the biggest overhead, and streaming the image should be pretty quick - but I can't see a reason why it needs to be synchronized.

ghansham commented 6 years ago

In JpegFormat and GifFormat classes, these are not synch methods and tiles are getting served faster. Try seq-Heat color pallette once. Its relatively slower for this palette. I dont know why.

guygriffiths commented 6 years ago

What sort of speed difference are you seeing?

jonblower commented 6 years ago

I wrote that a long time ago so I can't quite remember. But I think that there was a reason - I seem to remember getting problems if the method is not synchronized. (When requesting multiple images at the same time the images would get corrupted.) I think that something in ImageIO.write() is not thread safe - maybe it creates a single ImageWriter object for PNGs, which is reused for all writes.

However, these problems may have been fixed in later versions of the Java API. It would be worth trying removing the synchronized call, or at least looking at the SDK source code to find out what's going on. Also, it's definitely not necessary to synchronize the whole method - perhaps only the call to ImageIO.write().

ghansham commented 6 years ago

Atleast 3 times in writeImage call

On Oct 6, 2017 6:30 PM, "Guy Griffiths" notifications@github.com wrote:

What sort of speed difference are you seeing?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/Reading-eScience-Centre/edal-java/issues/97#issuecomment-334747731, or mute the thread https://github.com/notifications/unsubscribe-auth/ADFypE89H_6c7BR7e8RkcJXn5UwApseRks5spiRbgaJpZM4PwYkS .

guygriffiths commented 6 years ago

Can you provide some numbers, and details of what you're comparing?

ghansham commented 6 years ago

I am using latest edal/ncwms release. And deployed it in tomcat 8.5 20 with jdk 8. And added a glob expression for 18 hdf5 files which have variables in mercator projection. Size of grids is 3200x3200. And I am using a plain openlayers2 client. I added System.currentTimeMillis before and after on getMap function in WmsServlet class to check time spent and found potentially the time duration variation is lying in writeImage function call. I placed System.currentTimeMillis on this call as well . I also had put the same logic around imageIO.write method but that didnt seem to be bottleneck. It seems like multiple threads waiting on this method. The timings are 300 ms and 1100 ms for seq-Greys-inv and seq-Heat. Caching was enabled. And I was just switching color palettes over the same region.

guygriffiths commented 6 years ago

OK, I'll look into it when I get time, but some quick internet search suggests that what @jonblower is saying is true - that too many unsynchronized requests will cause image artifacts.

ghansham commented 6 years ago

My major concern is why it is taking a lot of time for some lut applied images and for some it is running quickly. And this is conclusion after running multiple times. I will have a look the way bufferedimage is being created.

jonblower commented 6 years ago

The problem may not be in how the bufferedimage is created (which is the same for all formats) but how it is encoded and pushed to the OutputStream. I have a feeling (which may be wrong or out of date) that the PNG encoder writes temporary files to disk, which might explain the lack of thread safety and the poor/variable performance.

I wonder how hard it would be to write a new, thread-safe PNG encoder? I expect that the hardest bit would be the zlib compression, which can probably be gotten from a library.

guygriffiths commented 6 years ago

I'm pretty sure the PNG encoder doesn't write to disk. I had a quick peek at the ImageIO code, and there's a cache option which seems to do what you suggest, but stepping through the code whilst running through ncWMS it decides (for whatever reason) that it shouldn't cache. I'll try turning off synchronization and making a bunch of simultaneous requests and see what happens...

guygriffiths commented 6 years ago

Hmm, it doesn't seem to cause any issues. I made 200 slightly different requests in quick succession, and all of the images came back fine. Perhaps it's been fixed since it was implemented? There's a discussion of it here: https://stackoverflow.com/questions/26297491/imageio-thread-safety where the answer with 4 votes says it's threadsafe and the one with 3 votes says it's not...

ghansham commented 6 years ago

I agree with Guy. But try different palettes with caching on and check do you have similar observations.

guygriffiths commented 6 years ago

So after trying even more simultaneous requests (800) and seeing no PNG artifacts, I think it's fairly safe to assume that the PNG encoder doesn't need synchronization any more.

As for the different palettes taking significantly longer to render, that's entirely down to the ImageIO.write() method - there's very little difference between different palettes with a JPEG output, but a noticeable change with PNG.

jonblower commented 6 years ago

I wonder where the bottleneck is with PNG - has anyone done any profiling? Sounds like something that might be worth optimising if the problem is significant.

ghansham commented 6 years ago

I have found all the palettes performance improving after removing synchronized..

On Oct 11, 2017 8:30 PM, "Jon Blower" notifications@github.com wrote:

I wonder where the bottleneck is with PNG - has anyone done any profiling? Sounds like something that might be worth optimising if the problem is significant.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/Reading-eScience-Centre/edal-java/issues/97#issuecomment-335840266, or mute the thread https://github.com/notifications/unsubscribe-auth/ADFypDZwrRQCS6HQM80JdwA_vBxUmhV4ks5srNgEgaJpZM4PwYkS .

ghansham commented 6 years ago

Hi..

So synchronized keyword will stay ultimately.

Ghansham

On Nov 1, 2017 21:14, "Guy Griffiths" notifications@github.com wrote:

Closed #97 https://github.com/Reading-eScience-Centre/edal-java/issues/97.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/Reading-eScience-Centre/edal-java/issues/97#event-1321248222, or mute the thread https://github.com/notifications/unsubscribe-auth/ADFypD-crz1tOoNEEdp7UH5r7rCT6iKhks5syJH0gaJpZM4PwYkS .

guygriffiths commented 6 years ago

No, it's been removed the develop branch. The next release won't have it.