Bug 51335 – [Qt][WK2] MappedMemoryPool is unbounded

• Status: RESOLVED
• Resolution: WONTFIX
• Component: WebKit2

[Balazs Kelemen]

The pool of mapped memory areas used by UpdateChunk's never release memory.
This means when you resize the window to a bigger size n times we are using n piece of big area while typically one medium size
would be enough for updates.

[Balazs Kelemen]

Created attachment 77009 [details]
Patch

[Balazs Kelemen]

Created attachment 77042 [details]
Patch

Eliminated the pointless base class, otherwise the same as the last one.

[Balazs Kelemen]

Comment on attachment 77042 [details]
Patch

First need to solve #51474 to avoid conflicts.

[Balazs Kelemen]

Created attachment 77989 [details]
Patch

[Kimmo Kinnunen]

This addresses a quite serious problem with MemoryMappedPool, good. However I'm not really convinced that anybody should spend that much time polishing current MemoryMappedPool before it really is working well.

To me, this patch just adds complexity on top of simple implementation. To me, address space running out is the least of the worries for MemoryMappedPool.

The simple implementation was initially done just to get WK2 on Qt working -- it is not meant as end-all design.

I opened another bug that discusses current problems:
https://bugs.webkit.org/show_bug.cgi?id=51984

Some of the problems are amplified by this patch. At least:
- this patch adds even more calls that don't check errors (file opens, mmaps)
- this patch adds polish (enums instead of bools)  to the isFree flag, which is incorrect anyway

[Balazs Kelemen]

Comment on attachment 77989 [details]
Patch

We should make a consensus about do
we really need the pooling 
optimization. Postpone this work until
it has been done.

[Kimmo Kinnunen]

(In reply to comment #6)
> (From update of attachment 77989 [details])
> We should make a consensus about do
> we really need the pooling 
> optimization. Postpone this work until
> it has been done.

Or, I can add that pooling if it makes you happy. Should I do that and do another patch?

[Balazs Kelemen]

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 77989 [details] [details])
> > We should make a consensus about do
> > we really need the pooling 
> > optimization. Postpone this work until
> > it has been done.
> 
> Or, I can add that pooling if it makes you happy. Should I do that and do another patch?

It's not about my happiness. If your experiences (benchmarks, profiling, 
user experience on device, etc.) does not justify the value of the optimization
then it should be removed. We can reintroduce it in the future if mmap will be a bottleneck.