Bug 148028 - [GTK] Re-add DiskImageCache
Summary: [GTK] Re-add DiskImageCache
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-08-14 09:43 PDT by Andre Moreira Magalhaes
Modified: 2016-01-04 05:42 PST (History)
5 users (show)

See Also:


Attachments
Patch (97.76 KB, patch)
2015-08-14 10:56 PDT, Andre Moreira Magalhaes
no flags Details | Formatted Diff | Diff
Patch (97.69 KB, patch)
2015-08-14 11:20 PDT, Andre Moreira Magalhaes
no flags Details | Formatted Diff | Diff
Patch (97.72 KB, patch)
2015-08-14 12:20 PDT, Andre Moreira Magalhaes
mcatanzaro: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andre Moreira Magalhaes 2015-08-14 09:43:13 PDT
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.
Comment 1 Andre Moreira Magalhaes 2015-08-14 10:56:25 PDT
Created attachment 259007 [details]
Patch
Comment 2 Andre Moreira Magalhaes 2015-08-14 11:20:51 PDT
Created attachment 259009 [details]
Patch
Comment 3 Andre Moreira Magalhaes 2015-08-14 12:20:34 PDT
Created attachment 259022 [details]
Patch
Comment 4 Andre Moreira Magalhaes 2015-09-15 10:00:08 PDT
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.
Comment 5 Andre Moreira Magalhaes 2015-09-15 10:45:25 PDT
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.
Comment 6 Carlos Garcia Campos 2015-09-16 00:34:31 PDT
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.
Comment 7 Andre Moreira Magalhaes 2015-09-16 08:59:22 PDT
(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.
Comment 8 Antti Koivisto 2015-09-16 13:51:14 PDT
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.
Comment 9 Andre Moreira Magalhaes 2015-09-17 09:57:30 PDT
(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.
Comment 10 Michael Catanzaro 2015-12-31 14:56:45 PST
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.
Comment 11 Andre Moreira Magalhaes 2016-01-04 05:42:12 PST
Thanks for the comments, closing this one and will open a separate report in case we decide to redo the implementation.