Bug 131096 – Pool IOSurfaces to help with allocation cost

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Tim Horton]

<rdar://problem/15373942>

[Tim Horton]

Created attachment 228377 [details]
WIP

[Tim Horton]

WIP patch, needs perf testing and probably tuning (and a changelog)

[Tim Horton]

Created attachment 228430 [details]
patch

[WebKit Commit Bot]

Attachment 228430 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/cg/IOSurfacePool.h:52:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/platform/graphics/cg/IOSurfacePool.h:52:  The parameter name "colorSpace" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tim Horton]

Created attachment 228431 [details]
patch

[WebKit Commit Bot]

Attachment 228431 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/cg/IOSurfacePool.h:52:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/platform/graphics/cg/IOSurfacePool.h:52:  The parameter name "colorSpace" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

Comment on attachment 228431 [details]
patch

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

> Source/WebCore/platform/graphics/cg/IOSurfacePool.cpp:108
> +        showPoolStatistics();

Do you really want all these showPoolStatistics calls?

> Source/WebCore/platform/graphics/cg/IOSurfacePool.cpp:112
> +    for (auto surfaceIter = mapIter->value.begin(); surfaceIter != mapIter->value.end(); ++surfaceIter) {

auto& ? I never know.

> Source/WebCore/platform/graphics/cg/IOSurfacePool.cpp:114
> +        RefPtr<IOSurface> surface = surfaceIter->get();
> +        if (!surfaceMatchesParameters(*surface, size, colorSpace))

Do we have to pay the ref() cost by making the refptr when we don't actually use the surface?

> Source/WebCore/platform/graphics/cg/IOSurfacePool.cpp:137
> +        RefPtr<IOSurface> surface = surfaceIter->get();
> +        if (!surfaceMatchesParameters(*surface, size, colorSpace))

Ditto.

> Source/WebCore/platform/graphics/cg/IOSurfacePool.h:91
> +    void scheduleCollectionTimer();
> +    void collectionTimerFired(Timer<IOSurfacePool>&);
> +    void collectInUseSurfaces();

Is this "collect" in the garbage collection sense? Not clear from the context.

[Tim Horton]

(In reply to comment #7)
> (From update of attachment 228431 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=228431&action=review
> 
> > Source/WebCore/platform/graphics/cg/IOSurfacePool.cpp:108
> > +        showPoolStatistics();
> 
> Do you really want all these showPoolStatistics calls?

I do, they're super useful for debugging. I would add a log channel but they're useful in release too. Maybe a macro that makes the call sites disappear if logging is disabled.

> > Source/WebCore/platform/graphics/cg/IOSurfacePool.cpp:112
> > +    for (auto surfaceIter = mapIter->value.begin(); surfaceIter != mapIter->value.end(); ++surfaceIter) {
> 
> auto& ? I never know.

Not with iterators I think (compiler complains).

> > Source/WebCore/platform/graphics/cg/IOSurfacePool.cpp:114
> > +        RefPtr<IOSurface> surface = surfaceIter->get();
> > +        if (!surfaceMatchesParameters(*surface, size, colorSpace))
> 
> Do we have to pay the ref() cost by making the refptr when we don't actually use the surface?

No! Will fix.

> > Source/WebCore/platform/graphics/cg/IOSurfacePool.cpp:137
> > +        RefPtr<IOSurface> surface = surfaceIter->get();
> > +        if (!surfaceMatchesParameters(*surface, size, colorSpace))
> 
> Ditto.
> 
> > Source/WebCore/platform/graphics/cg/IOSurfacePool.h:91
> > +    void scheduleCollectionTimer();
> > +    void collectionTimerFired(Timer<IOSurfacePool>&);
> > +    void collectInUseSurfaces();
> 
> Is this "collect" in the garbage collection sense? Not clear from the context.

I don't exactly know what to call it. It was at some point "collectInUseSurfacesAndMarkUnusedSurfacesPurgeable" but that is very long. And "collect" meant "move surfaces that aren't in-use anymore from the in-use pool to the unused pool".

[Tim Horton]

http://trac.webkit.org/changeset/166682

[Sam Weinig]

> > > Source/WebCore/platform/graphics/cg/IOSurfacePool.cpp:112
> > > +    for (auto surfaceIter = mapIter->value.begin(); surfaceIter != mapIter->value.end(); ++surfaceIter) {
> > 
> > auto& ? I never know.
> 
> Not with iterators I think (compiler complains).

Why not:
    for (auto& surface : mapIter->value) {
    }

[Tim Horton]

(In reply to comment #10)
> > > > Source/WebCore/platform/graphics/cg/IOSurfacePool.cpp:112
> > > > +    for (auto surfaceIter = mapIter->value.begin(); surfaceIter != mapIter->value.end(); ++surfaceIter) {
> > > 
> > > auto& ? I never know.
> > 
> > Not with iterators I think (compiler complains).
> 
> Why not:
>     for (auto& surface : mapIter->value) {
>     }

Because Deque::remove takes an iterator? Is there a good way to make this work with range-based for?

[Simon Fraser (smfr)]

> I don't exactly know what to call it. It was at some point "collectInUseSurfacesAndMarkUnusedSurfacesPurgeable" but that is very long.

You could have put that in a comment.