Bug 148028 – [GTK] Re-add DiskImageCache

• Status: RESOLVED
• Resolution: WONTFIX
• Component: WebKitGTK

[Andre Moreira Magalhaes]

DiskImageCache is used in the rpi port of webkit but the original work was removed in changeset r173265. 

This bug is about re-adding the support for GTK only but including the changes from bug #124167 that would allow adding support for other platforms easily. This would help maintaining the rpi port without having to carry the patch separately.

Patch to follow.

[Andre Moreira Magalhaes]

Created attachment 259007 [details]
Patch

[Andre Moreira Magalhaes]

Created attachment 259009 [details]
Patch

[Andre Moreira Magalhaes]

Created attachment 259022 [details]
Patch

[Andre Moreira Magalhaes]

Hi, is upstream interested in this patch? We would love to have this merged if possible. Please let us know and we can tweak the patch as needed.

[Andre Moreira Magalhaes]

To clarify, the DiskImageCache differs from the normal disk cache by storing decoded image data on a mmaped file while the normal disk cache stores downloaded resources (ie. the png/jpeg stream), thus avoiding having to decode images already in cache again.

[Carlos Garcia Campos]

I didn't know this DiskImageCache. So, the idea is to cache decoded images in disk temporary, because reading from disk is faster than decoding, right? I wonder why it was removed from the IOS port.

[Andre Moreira Magalhaes]

(In reply to comment #6)
> I didn't know this DiskImageCache. So, the idea is to cache decoded images
> in disk temporary, because reading from disk is faster than decoding, right?
> I wonder why it was removed from the IOS port.

Yes, you are correct, and the cached data is mmaped also. 

From changeset r173265:
"Disk image cache code unnecessarily complicates SharedBuffer implementation. We can remove this now since we don't enable it in WebKit2 on iOS."

I guess as the code was being used only in iOS and they were not enabling it on webkit2, they just removed it to simplify the SharedBuffer impl. Thing is this is still useful on the rpi and we made the code cross platform (see bug #124167), although enabled only on the gtk port.

[Antti Koivisto]

I don't think we should bring this code back as it is pretty invasive and may interfere with other improvements we may want to make. The right way to do this would be to expand the network cache to support derived data in cache entries. For images this could be decoded bitmaps, for js bytecode etc.

[Andre Moreira Magalhaes]

(In reply to comment #8)
> I don't think we should bring this code back as it is pretty invasive and
> may interfere with other improvements we may want to make. The right way to
> do this would be to expand the network cache to support derived data in
> cache entries. For images this could be decoded bitmaps, for js bytecode etc.

Thanks for the feedback. Could you please elaborate a bit more on the worries about this approach? Why do you think it is too invasive? The changes to CachedImage/SharedBuffer and co are not that big and I believe we would need a similar approach if using a modified network cache for that.

In any case I will check about the network cache approach.

[Michael Catanzaro]

Comment on attachment 259022 [details]
Patch

To get this upstream, I think you'll need to redo the implementation as Antti suggested, sorry.

But I'm not sure if we really want this upstream at all, to be honest, if it's only a performance win on specialized hardware.

[Andre Moreira Magalhaes]

Thanks for the comments, closing this one and will open a separate report in case we decide to redo the implementation.