Bug 207374 - [Cocoa] Slightly improve performance of Font::getCFStringAttributes()
Summary: [Cocoa] Slightly improve performance of Font::getCFStringAttributes()
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: Myles C. Maxfield
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-02-06 19:23 PST by Myles C. Maxfield
Modified: 2020-02-07 12:27 PST (History)
7 users (show)

See Also:


Attachments
Patch (4.66 KB, patch)
2020-02-06 19:25 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (4.66 KB, patch)
2020-02-06 19:30 PST, Myles C. Maxfield
darin: review+
Details | Formatted Diff | Diff
Patch for committing (4.73 KB, patch)
2020-02-07 10:30 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2020-02-06 19:23:15 PST
[Cocoa] Slightly improve performance of Font::getCFStringAttributes()
Comment 1 Myles C. Maxfield 2020-02-06 19:25:32 PST
Created attachment 390046 [details]
Patch
Comment 2 Myles C. Maxfield 2020-02-06 19:30:58 PST
Created attachment 390048 [details]
Patch
Comment 3 Darin Adler 2020-02-07 09:26:14 PST
Comment on attachment 390048 [details]
Patch

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

This looks good. Like I said earlier, if this is *really* hot then we should go even further with some kind of caching/memoizing so we don’t create a dictionary at all in most cases.

> Source/WebCore/platform/graphics/mac/SimpleFontDataCoreText.cpp:38
> +    CFTypeRef keys[5];
> +    CFTypeRef values[5];

Easy mistake would be to add a new case below and not update the array sizes here. Would be nice if we could find a way to assert or static_assert or something to make it trivial to catch the mistake either at compile time or runtime.

> Source/WebCore/platform/graphics/mac/SimpleFontDataCoreText.cpp:59
> +    static CTParagraphStyleRef paragraphStyle = nullptr;
> +    if (!paragraphStyle) {
> +        paragraphStyle = CTParagraphStyleCreate(nullptr, 0);
> +        CTParagraphStyleSetCompositionLanguage(paragraphStyle, kCTCompositionLanguageNone);
> +    }

This can be written something like this:

    static CTParagraphStyleRef paragraphStyle = [] () {
        auto style = CTParagraphStyleCreate(nullptr, 0);
        CGParagraphStyleSetCompositionLanguage(style, kCTCompositionLanguageNone);
        return style;
     }();

I think it’s slightly better coding style. Could even put this into a separate function for clarity.
Comment 4 Myles C. Maxfield 2020-02-07 10:30:50 PST
Created attachment 390103 [details]
Patch for committing
Comment 5 WebKit Commit Bot 2020-02-07 10:59:15 PST
Comment on attachment 390103 [details]
Patch for committing

Clearing flags on attachment: 390103

Committed r256037: <https://trac.webkit.org/changeset/256037>
Comment 6 Radar WebKit Bug Importer 2020-02-07 12:27:17 PST
<rdar://problem/59269074>