GNOME Bugzilla – Bug 667682
Cache size dramatically exceeds limit
Last modified: 2014-12-07 13:48:53 UTC
Hi, I recently discover that despite i was using the inside functionality to manage cookies, temporary downladed files, etc.. from epiphany; the cache folder of the application was never cleaned up In fact that folder at least 190Mo and over that 10000 elements What could be the reason? Is the managing personal data functionality normally able to manage this folder? Should i delete this folder manually to make it work normally? Xavier
I just tried this with 3.3.x I had a small cache, recently cleaned, which was 9Mb. The Clean button in Preferences → Privacy reduced it to 3Mb. Perhaps it retained the cache files relevant to currently opened tabs. Would you check again with a newer Epiphany? At least 3.2.
In my distribution (Fedora 15) which i follow in order to keep easy my using, my version is only 3.03. Can i help you on that version anyway?
I can confirm this. Ephy is leaking disk cache, perhaps when it crashes. This has been the case at least since the cache directory was moved out of .gnome2/epiphany. Just now, "du -hs .cache/epiphany" reported 4.5G, hitting the Clean button reduced that to 4.3G. My cache size is set to 256MB in the prefs. Currently, the most common way Ephy exits for me is either because of a crash, I accidentally hit Ctrl-Q, or I have to kill it. This happens at least once a week, so is perhaps a candidate reason for this unbounded disk cache growth. Epiphany 3.4.1 WebkitGtk 1.8.0 Ubuntu Precise
On my machine, the Epiphany cache ends up taking 12GB. That's far too much, and is going to be a problem on machines with small amounts of disk (netbooks, tablets with 16GB or 32GB of SSD).
This is especially a problem when the "disk cache" option in the Preferences is set to "50 MB".
(In reply to comment #5) > This is especially a problem when the "disk cache" option in the Preferences is > set to "50 MB". This preference isn't hooked up to anything, and WebKit does not provide any way to control the size of the cache. :/
(In reply to comment #6) > (In reply to comment #5) > > This is especially a problem when the "disk cache" option in the Preferences is > > set to "50 MB". > > This preference isn't hooked up to anything, and WebKit does not provide any > way to control the size of the cache. :/ I think that would be: NetworkProcess::platformSetCacheModel() in Source/WebKit2/NetworkProcess/soup/NetworkProcessSoup.cpp It only uses the free disk space and the memory size to calculate the max cache size.
(In reply to comment #7) > (In reply to comment #6) > > (In reply to comment #5) > > > This is especially a problem when the "disk cache" option in the Preferences is > > > set to "50 MB". > > > > This preference isn't hooked up to anything, and WebKit does not provide any > > way to control the size of the cache. :/ > > I think that would be: > NetworkProcess::platformSetCacheModel() > in Source/WebKit2/NetworkProcess/soup/NetworkProcessSoup.cpp > > It only uses the free disk space and the memory size to calculate the max cache > size. Yes, it's calculated automatically depending on the cache model. But we can provide API to override that.
Well either I am seriously misreading calculateCacheSizes (in Source/WebKit2/Shared/CacheModel.cpp), or else 12 GB is way over the max limit regardless (175 MiB). So we have two bugs: 1) Setting exists in preferences, but isn't hooked up 2) Cache size wildly exceeds the computed limit I think we do not need or want a preference for cache size in Epiphany. If the cache size was really limited to 175 MiB, I doubt anyone would care to change it.
Could it be that, after a crash, or if web/network process exits without calling soup_cache_dump() for whatever reason, we end up with files in the cache dir not referenced from the soup cache file?
(In reply to comment #10) > Could it be that, after a crash, or if web/network process exits without > calling soup_cache_dump() for whatever reason, we end up with files in the > cache dir not referenced from the soup cache file? Carlos is right. Due to the way the cache works, it's mandatory to call soup_cache_dump() in order to dump the cache index to disk. The advantage is that we reduce disk writes but a big disadvantage which is that we loose the info of the cache (and "leak" the stored resources) if not called. A "quick" fix that comes to my mind would be to mark the index of the cache as dirty just after loading it. That dirty flag would be unset by soup_cache_dump(). Should we find the dirty bit when reading the index, means that we must clear the contents of the cache and the index. WDYT? The "good" fix would be to have the index stored in some writable mmap'ed portion of memory so the OS would take care of the updates to disk.
Wrote a simple patch for libsoup to get the leaked entries in my cache dir and got this: 14670 entries (958966651 bytes total) leaked in /home/cgarcia/.cache/epiphany $ du -b /home/cgarcia/.cache/epiphany 1059909993 /home/cgarcia/.cache/epiphany So the real cache size here is 100943342 bytes (~96MB), and there's almost 1G leaked.
(In reply to comment #11) > A "quick" fix that comes to my mind would be to mark the index of the cache as > dirty just after loading it. That dirty flag would be unset by > soup_cache_dump(). Should we find the dirty bit when reading the index, means > that we must clear the contents of the cache and the index. WDYT? > > The "good" fix would be to have the index stored in some writable mmap'ed > portion of memory so the OS would take care of the updates to disk. I think any user of SoupCache would need to do one or the other, or else never ever crash. Maybe libsoup should automatically take care of cleaning a dirty cache, or have an option for doing so? Dan?
Well as far as I can tell, this will have to be fixed in libsoup....
Created attachment 287367 [details] [review] cache: Clean up leaked resources when loading the cache
(In reply to comment #15) > Created an attachment (id=287367) [details] [review] > cache: Clean up leaked resources when loading the cache The idea is good but the problem I see with the patch is that it's adding a g_unlink for every single _wrap_input() call, something that is likely adding a context switch plus an I/O operation which will likely degrade performance. We should try to minimize the amount of I/O as much as possible. BTW awesome test case, the cache really needs more testing!
Yes, actually I used the inverted logic, using dumped file, instead of a locked file for several reasons: a) To make sure the first time load is called again all existing leaks will be removed for everybody. b) In case soup_load is not called for whatever reason c) To handle also the case when the cache is loaded, nothing is updated and not dumped. There aren't leaks in this case even if dump hasn't been called. a) could be solved by bumping the index version even if the index format hasn't changed, to force a clean cache. b) is an api misuse according to sergio and c) is probably a situation that hardly ever happens. So, what do you think about creating a lock file in load, deleted by dump and bump the index version to force a clean cache? Regarding b), if it's a programmer error, shouldn't we ensure it somehow? making other api methods fail if load hasn't be called or something like that? Note that non of the existing tests use soup_cache_load at all, that's why I assumed it was not mandatory.
I've noticed another problem of using the lock approach. There isn't a 1:1 relationship between load and dump, because dump could be called multiple times. So, for example the case I'm testing in the unit test wouldn't work: 1.- load() <some resources are cached> 2.- dump() <more resources are cached> <crash> The first dump removed the lock, so the resources cached after the dump are crashed. Since the only concern of the current approach is the unlink call, I'll use a variable to ensure that unlink is only called for the first cached resource after dump
Created attachment 287575 [details] [review] Updated patch Make sure we don't call unlink for every cached resource, but only for the first one cached after dump().
Comment on attachment 287575 [details] [review] Updated patch Looks good to me. >+ if (!priv->dumped_file_exists) { >+ fd = g_open (priv->dumped_filename, O_CREAT | O_EXCL, 0600); >+ if (fd != -1) { >+ priv->dumped_file_exists = TRUE; >+ close (fd); >+ } >+ } another possibility would be if (!priv->dumped_file_exists) priv->dumped_file_exists = g_file_set_contents (priv->dumped_filename, "", 0, NULL); (which would do more work internally, but is simpler here...) >+ else if (leaked_entries) { >+ char *key_str = g_strdup_printf ("%u", entry->key); >+ >+ /* Not a leaked entry */ >+ g_hash_table_remove (leaked_entries, key_str); Hm... it would be easier to use the guint32 as the hash table key directly so you didn't have to create a temporary string here...
Thinking again about this, I wonder if it wouldn't be easier to simply check for leaks on every cache load unconditionally. Note that it's a read dir, we don't actually need to read every file, only the directory to get the filenames, and it only happens once when the cache is loaded. It shouldn't be slow, and creating and checking the dumped file also adds IO and makes the code a lot more complex and racy too.
(In reply to comment #21) > Thinking again about this, I wonder if it wouldn't be easier to simply check > for leaks on every cache load unconditionally. Note that it's a read dir, we > don't actually need to read every file, only the directory to get the > filenames, and it only happens once when the cache is loaded. It shouldn't be > slow, and creating and checking the dumped file also adds IO and makes the code > a lot more complex and racy too. I think it's worth trying.
Created attachment 292173 [details] [review] Updated patch Removed all the dumped file mess, and simplified everything. Also added a helper soup_cache_file_foreach function to use it also by clear_cache_files().
Comment on attachment 292173 [details] [review] Updated patch Sure, makes sense.
Comment on attachment 292173 [details] [review] Updated patch Pushed