Bug 212918 - Extended Color: Switch ColorMac.mm's nsColor() function over to using TinyLRUCache
Summary: Extended Color: Switch ColorMac.mm's nsColor() function over to using TinyLRU...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sam Weinig
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-06-08 13:43 PDT by Sam Weinig
Modified: 2020-06-09 07:14 PDT (History)
2 users (show)

See Also:


Attachments
Patch (5.14 KB, patch)
2020-06-08 13:47 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (7.30 KB, patch)
2020-06-08 16:29 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (7.27 KB, patch)
2020-06-08 18:45 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2020-06-08 13:43:58 PDT
Extended Color: Switch ColorMac.mm's nsColor() function over to using TinyLRUCache
Comment 1 Sam Weinig 2020-06-08 13:47:15 PDT
Created attachment 401366 [details]
Patch
Comment 2 Darin Adler 2020-06-08 14:11:13 PDT
Comment on attachment 401366 [details]
Patch

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

> Source/WebCore/platform/graphics/Color.h:192
> +    bool isExtended() const { return !(m_colorData.simpleColorAndFlags & invalidSimpleColor); }

Not sure there is a benefit to this, but I think this can be constexpr.

> Source/WebCore/platform/graphics/Color.h:195
> +    const SimpleColor asSimple() const;

We might even be able to make this a constexpr if we are willing to move it to the header.

> Source/WebCore/platform/graphics/mac/ColorMac.mm:41
> +    return [NSColor colorWithCGColor:cachedCGColor(color)];

Is there a problem with populating both tiny cache each time? I guess not.
Comment 3 Darin Adler 2020-06-08 14:11:34 PDT
Comment on attachment 401366 [details]
Patch

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

>> Source/WebCore/platform/graphics/Color.h:192
>> +    bool isExtended() const { return !(m_colorData.simpleColorAndFlags & invalidSimpleColor); }
> 
> Not sure there is a benefit to this, but I think this can be constexpr.

Really tempting to also add isSimple().
Comment 4 Sam Weinig 2020-06-08 14:50:56 PDT
(In reply to Darin Adler from comment #2)
> Comment on attachment 401366 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=401366&action=review
> 
> > Source/WebCore/platform/graphics/Color.h:192
> > +    bool isExtended() const { return !(m_colorData.simpleColorAndFlags & invalidSimpleColor); }
> 
> Not sure there is a benefit to this, but I think this can be constexpr.
> 

Currently, you can't make a constexpr Color at all, if I remember correctly due to the way the m_colorData union is initialized (though I might be misremembering this).
Comment 5 Sam Weinig 2020-06-08 16:29:45 PDT
Created attachment 401397 [details]
Patch
Comment 6 Darin Adler 2020-06-08 16:33:03 PDT
Comment on attachment 401397 [details]
Patch

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

> Source/WebCore/platform/graphics/mac/ColorMac.mm:142
> +    ASSERT(color.isExtended() || color.asSimple().value());

Do we need to write this assertion? Will TinyLRUCache automatically assert the same thing for us?

> Source/WebCore/platform/graphics/mac/ColorMac.mm:145
> +    return cache.get().get(color).get();

"get, get, get"

Better than "kvetch, kvetch, kvetch" I guess.
Comment 7 Sam Weinig 2020-06-08 18:28:24 PDT
(In reply to Darin Adler from comment #6)
> Comment on attachment 401397 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=401397&action=review
> 
> > Source/WebCore/platform/graphics/mac/ColorMac.mm:142
> > +    ASSERT(color.isExtended() || color.asSimple().value());
> 
> Do we need to write this assertion? Will TinyLRUCache automatically assert
> the same thing for us?

Yeah, it will, by virtue of calling cachedCGColor() which is where I copied this assert from. I am actually not sure it is necessary in either place anymore.

> 
> > Source/WebCore/platform/graphics/mac/ColorMac.mm:145
> > +    return cache.get().get(color).get();
> 
> "get, get, get"
> 
> Better than "kvetch, kvetch, kvetch" I guess.
Comment 8 Sam Weinig 2020-06-08 18:45:09 PDT
Created attachment 401406 [details]
Patch
Comment 9 EWS 2020-06-09 07:13:39 PDT
Committed r262790: <https://trac.webkit.org/changeset/262790>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 401406 [details].
Comment 10 Radar WebKit Bug Importer 2020-06-09 07:14:21 PDT
<rdar://problem/64162829>