Bug 68434 – [GTK] Implement cache model for WebKit2

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Carlos Garcia Campos]

It's currently not implemented and there isn't GTK+ API exposing it either.

[Carlos Garcia Campos]

Created attachment 107992 [details]
Patch

It was already rejected by Martin in bug #67931.

[Alejandro G. Castro]

Comment on attachment 107992 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=107992&action=review

> Source/WebKit2/WebProcess/gtk/WebProcessGtk.cpp:52
> +    // FIXME: The Mac port calculates these values based on the amount of physical memory that's
> +    // installed on the system. Currently these values match the Mac port for users with more than
> +    // 512 MB and less than 1024 MB of physical memory.

This comment does not apply anymore here because it was used for the webkit1 selection of magical values we used for the cache. If we are using the heuristics in the calculateCacheSizes function we should check if they are ok with us. I think we could active them for wk2 and check if it is doing right or wrong.

[Carlos Garcia Campos]

Created attachment 108325 [details]
Patch

Patch implementing the FIXMEs

[Martin Robinson]

Comment on attachment 108325 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=108325&action=review

Looks good to me. We just need another reviewer to approve the API.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:83
> + * determine its caching behavior. All web views follow the cache

Should probably say all #WebkitWebViews in  #WebKitWebContext follow the cache model

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:85
> + * for caching previously viewed content .

Extra space after 'content'

> Source/WebKit2/WebProcess/gtk/WebProcessGtk.cpp:98
> +    SoupSession* session = WebCore::ResourceHandle::defaultSession();
> +    SoupCache* cache = reinterpret_cast<SoupCache*>(soup_session_get_feature(session, SOUP_TYPE_CACHE));

Might as well move these two down above:  if (cache)

[Alejandro G. Castro]

Comment on attachment 108325 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=108325&action=review

> Source/WebKit2/WebProcess/gtk/WebProcessGtk.cpp:71
> +    guint64 totalMem = g_ascii_strtoull(ptr + strlen("MemTotal:"), &endPtr, 10);

Could we have strlen("MemTotal:") precalculated in a static variable? Maybe we could even use sysconf with _SC_PHYS_PAGES and _SC_PAGE_SIZE, it could be cleaner.

[Carlos Garcia Campos]

(In reply to comment #4)
> (From update of attachment 108325 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=108325&action=review
> 
> Looks good to me. We just need another reviewer to approve the API.
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:83
> > + * determine its caching behavior. All web views follow the cache
> 
> Should probably say all #WebkitWebViews in  #WebKitWebContext follow the cache model

Ok.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:85
> > + * for caching previously viewed content .
> 
> Extra space after 'content'

Ok.

> > Source/WebKit2/WebProcess/gtk/WebProcessGtk.cpp:98
> > +    SoupSession* session = WebCore::ResourceHandle::defaultSession();
> > +    SoupCache* cache = reinterpret_cast<SoupCache*>(soup_session_get_feature(session, SOUP_TYPE_CACHE));
> 
> Might as well move these two down above:  if (cache)

I need the cache to get the free disk space:

uint64_t diskFreeSize = getCacheDiskFreeSize(cache);

[Carlos Garcia Campos]

(In reply to comment #5)
> (From update of attachment 108325 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=108325&action=review
> 
> > Source/WebKit2/WebProcess/gtk/WebProcessGtk.cpp:71
> > +    guint64 totalMem = g_ascii_strtoull(ptr + strlen("MemTotal:"), &endPtr, 10);
> 
> Could we have strlen("MemTotal:") precalculated in a static variable?

This code will usually be executed only once to set the cache model, so I don't think we need micro-optimizations here, but if you think it's worh it I don't mind to do it.

> Maybe we could even use sysconf with _SC_PHYS_PAGES and _SC_PAGE_SIZE, it could be cleaner.

__get_phys_pages() (called by sysconf to get _SC_PHYS_PAGES) parses /proc/meminfo to get the total mem, and returns total_mem / (__getpagesize () / 1024). We are doing the same, but getting the value directly. I agree using sysconf might be cleaner, though

[Martin Robinson]

Comment on attachment 108325 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=108325&action=review

> Source/WebKit2/WebProcess/gtk/WebProcessGtk.cpp:54
> +    g_object_get(G_OBJECT(cache), "cache-dir", &cacheDir.outPtr(), NULL);
> +    if (!cacheDir)
> +        return 0;

Would it make sense to attach a listener to this property to update WebCore when it changes?

>>> Source/WebKit2/WebProcess/gtk/WebProcessGtk.cpp:71
>>> +    guint64 totalMem = g_ascii_strtoull(ptr + strlen("MemTotal:"), &endPtr, 10);
>> 
>> Could we have strlen("MemTotal:") precalculated in a static variable? Maybe we could even use sysconf with _SC_PHYS_PAGES and _SC_PAGE_SIZE, it could be cleaner.
> 
> This code will usually be executed only once to set the cache model, so I don't think we need micro-optimizations here, but if you think it's worh it I don't mind to do it.

Sysconf seems like it would be a lot cleaner and not Linux only. I think it'd be better to ues that here if it makes sense to you.

[Carlos Garcia Campos]

(In reply to comment #8)
> (From update of attachment 108325 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=108325&action=review
> 
> > Source/WebKit2/WebProcess/gtk/WebProcessGtk.cpp:54
> > +    g_object_get(G_OBJECT(cache), "cache-dir", &cacheDir.outPtr(), NULL);
> > +    if (!cacheDir)
> > +        return 0;
> 
> Would it make sense to attach a listener to this property to update WebCore when it changes?

To update what? 

> >>> Source/WebKit2/WebProcess/gtk/WebProcessGtk.cpp:71
> >>> +    guint64 totalMem = g_ascii_strtoull(ptr + strlen("MemTotal:"), &endPtr, 10);
> >> 
> >> Could we have strlen("MemTotal:") precalculated in a static variable? Maybe we could even use sysconf with _SC_PHYS_PAGES and _SC_PAGE_SIZE, it could be cleaner.
> > 
> > This code will usually be executed only once to set the cache model, so I don't think we need micro-optimizations here, but if you think it's worh it I don't mind to do it.
> 
> Sysconf seems like it would be a lot cleaner and not Linux only. I think it'd be better to ues that here if it makes sense to you.

Ok, I'll use Sysconf then.

[Carlos Garcia Campos]

Created attachment 109934 [details]
Updated patch to use sysconf

[Martin Robinson]

Comment on attachment 109934 [details]
Updated patch to use sysconf

View in context: https://bugs.webkit.org/attachment.cgi?id=109934&action=review

Looks good, but please handle the situation where sysconf returns an error before landing.

> Source/WebKit2/WebProcess/gtk/WebProcessGtk.cpp:46
> +#else
> +static uint64_t kDefaultMemorySize = 512;
> +#endif

I would move this down to getMemorySize.

> Source/WebKit2/WebProcess/gtk/WebProcessGtk.cpp:68
> +#if !OS(WINDOWS)
> +    return ((sysconf(_SC_PAGESIZE) / 1024) * sysconf(_SC_PHYS_PAGES)) / 1024;
> +#else
> +    // Fallback to default for other platforms.

If any of these sysconf calls return -1 you need to return the default memory size.

[Carlos Garcia Campos]

(In reply to comment #11)
> (From update of attachment 109934 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=109934&action=review
> 
> Looks good, but please handle the situation where sysconf returns an error before landing.
> 
> > Source/WebKit2/WebProcess/gtk/WebProcessGtk.cpp:46
> > +#else
> > +static uint64_t kDefaultMemorySize = 512;
> > +#endif
> 
> I would move this down to getMemorySize.

Ok.

> > Source/WebKit2/WebProcess/gtk/WebProcessGtk.cpp:68
> > +#if !OS(WINDOWS)
> > +    return ((sysconf(_SC_PAGESIZE) / 1024) * sysconf(_SC_PHYS_PAGES)) / 1024;
> > +#else
> > +    // Fallback to default for other platforms.
> 
> If any of these sysconf calls return -1 you need to return the default memory size.

Right! Btw, this patch depends on bug #69506, it uses WEBKIT_API instead of WK_EXPORT

[Carlos Garcia Campos]

Committed r96829: <http://trac.webkit.org/changeset/96829>