Bug 47226 – Cache CGColor as we do NSColor

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Darin Adler]

Cache CGColor as we do NSColor

[Darin Adler]

Created attachment 69866 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 69866 [details]
Patch

+        This fixes performance problems on certain web pages that use

I assume you can't mention an example. Can a manual test be made?

+			developmentRegion = English;

Please don't land this.


-    CGColorRef debugColor = createCGColor(Color(255, 0, 0, 204));
-    m_rootLayer->setBackgroundColor(debugColor);
-    CGColorRelease(debugColor);
+    m_rootLayer->setBackgroundColor(cachedCGColor(Color(255, 0, 0, 204), DeviceColorSpace));

Even though it's inside ifdef NDEBUG, the "debugColor" name seemed helpful. And I don't think that we should be caching it.


     if (m_owner->showDebugBorders()) {
-        CGColorRef borderColor = createCGColor(Color(128, 0, 128, 180));
-        CACFLayerSetBorderColor(newLayer.get(), borderColor);
-        CGColorRelease(borderColor);
+        CACFLayerSetBorderColor(newLayer.get(), cachedCGColor(Color(128, 0, 128, 180), DeviceColorSpace));

Ditto.

I don't know a lo about CGColor, but since this just applies the same technique that we used with NSColors, r=me.

I'm wondering why we can't store the platform color inside Color, and have to iterate over the vector in a disconnected cache. Are there so many Color objects that it's a memory use issue? Or are they transient, so storing the CGColor inside does no good?

[Darin Adler]

(In reply to comment #2)
> (From update of attachment 69866 [details])
> +        This fixes performance problems on certain web pages that use
> 
> I assume you can't mention an example. Can a manual test be made?

I could mention an example.

> +            developmentRegion = English;
> 
> Please don't land this.

Are you sure? Now that everyone can get the new Xcode, maybe we should start checking this in. This was different before because many Xcode users didn’t have access to the new Xcode, but now everyone can get it from Software Update.

> -    CGColorRef debugColor = createCGColor(Color(255, 0, 0, 204));
> -    m_rootLayer->setBackgroundColor(debugColor);
> -    CGColorRelease(debugColor);
> +    m_rootLayer->setBackgroundColor(cachedCGColor(Color(255, 0, 0, 204), DeviceColorSpace));
> 
> Even though it's inside ifdef NDEBUG, the "debugColor" name seemed helpful. And I don't think that we should be caching it.

I’d be happy to add the named local variable back for documentation/clarity purposes.

But I don’t agree about the caching. After my patch don’t have a non-cached API for making CG colors out of Color objects and I don’t think we need a non-cached alternative. I chose to put “cached” in the name of the function so it woud be easier to understand there is no need to CFRelease the function result, but that might put undue emphasis on the caching.

>      if (m_owner->showDebugBorders()) {
> -        CGColorRef borderColor = createCGColor(Color(128, 0, 128, 180));
> -        CACFLayerSetBorderColor(newLayer.get(), borderColor);
> -        CGColorRelease(borderColor);
> +        CACFLayerSetBorderColor(newLayer.get(), cachedCGColor(Color(128, 0, 128, 180), DeviceColorSpace));
> 
> Ditto.

Sure I’ll add the name back.

> I'm wondering why we can't store the platform color inside Color, and have to iterate over the vector in a disconnected cache. Are there so many Color objects that it's a memory use issue? Or are they transient, so storing the CGColor inside does no good?

Color is simply an RGB value plus a validity boolean. I don’t think we should change it into a larger object that holds platform color handles as well. The cache is shared between multiple Color objects, so adding a platform handle wouldn’t eliminate the need for a cache.

I suppose the answer to one of your questions is yes. They are transient.

[Alexey Proskuryakov]

> > +            developmentRegion = English;
> > 
> > Please don't land this.
> 
> Are you sure? Now that everyone can get the new Xcode, maybe we should start checking this in.

We should probably communicate this on webkit-dev. Also, I'm not sure how many frequent contributors are still on Leopard.

> But I don’t agree about the caching.

I think it's moderately valuable to have the same colors in the cache in both debug and release builds. It seems that these are the only two functions that break it.

[Darin Adler]

Committed r69259: <http://trac.webkit.org/changeset/69259>

[Simon Fraser (smfr)]

Do you have more information about the performance issue? What kind of content show it?