<rdar://problem/15373942>
Created attachment 228377 [details] WIP
WIP patch, needs perf testing and probably tuning (and a changelog)
Created attachment 228430 [details] patch
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.
Created attachment 228431 [details] patch
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.
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.
(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".
http://trac.webkit.org/changeset/166682
> > > 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) { }
(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?
> 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.