Improve perf for CTFramesetterSuggestFrameSizeWithConstraints #1603

aballway · 2016-12-21T19:05:25Z

CTFramesetterSuggestFrameSizeWithConstraints was going through the entire process of creating a frame to get the size, which was very wasteful. Now only does the minimum amount of layout to determine the suggested size. Cleans up NSString+UIKitAdditions for better perf analysis.

This change is

msft-Jeyaram · 2016-12-21T19:36:15Z

Frameworks/CoreText/CTFramesetter.mm

-
-    [frame release];
-    return ret;
+    return framesetter ?


ramesetter ? [](start = 12, length = 12)

LOL #Closed

msft-Jeyaram · 2016-12-21T19:37:49Z

Frameworks/CoreText/CTFramesetter.mm

-    [frame release];
-    return ret;
+    return framesetter ?
+               _DWriteGetSize((CFAttributedStringRef) static_cast<_CTFramesetter*>(framesetter)->_typesetter->_attributedString.get(),


ingRef) static_cast<_CTFra [](start = 46, length = 26)

mix of type casting conventions. #Closed

msft-Jeyaram · 2016-12-21T19:44:18Z

Frameworks/CoreText/DWriteWrapper_CoreText.mm

@@ -522,4 +523,56 @@ HRESULT STDMETHODCALLTYPE GetPixelsPerDip(_In_opt_ void* clientDrawingContext, _
    }

    return frame;
+}
+
+static CGSize _DWriteGetSize(CFAttributedStringRef string, CFRange range, CGSize maxSize, CFRange* fitRange) {


CFRange [](start = 59, length = 7)

github id

msft-Jeyaram · 2016-12-21T19:50:57Z

Frameworks/CoreText/DWriteWrapper_CoreText.mm

+        uint32_t lineCount = 0;
+
+        // Ignore return value, we only want the lineCount for now
+        textLayout->GetLineMetrics(nullptr, 0, &lineCount);


textLayout->GetLineMetrics(nullptr, 0, &lineCount); [](start = 7, length = 52)

is this guaranteed to return the correct line count if the function fails? #Closed

yup #Closed

tried looking at the documentation could not find it :(.
any insights?

In reply to: 93508293 [](ancestors = 93508293)

It's in the documentation

see https://msdn.microsoft.com/en-us/library/windows/desktop/dd316763(v=vs.85).aspx

" If maxLineCount is not large enough E_NOT_SUFFICIENT_BUFFER, which is equivalent to HRESULT_FROM_WIN32(ERROR_INSUFFICIENT_BUFFER), is returned and *actualLineCount is set to the number of lines needed. "

In reply to: 93508458 [](ancestors = 93508458,93508293)

I saw that, but that is only if E_NOT_SUFFICIENT_BUFFER is returned as the HRESULT, it doesn't specify for other errors.

In reply to: 93509971 [](ancestors = 93509971,93508458,93508293)

safer to check for that exact case (also good for unwanted behavior)
Given the implementation and documentation only mention it for the specific error above.
e.g someone decides to change the LINE_METRICS to and invalid state, this would help catch the error faster.

In reply to: 93510430 [](ancestors = 93510430,93509971,93508458,93508293)

msft-Jeyaram · 2016-12-21T19:57:59Z

Frameworks/CoreText/DWriteWrapper_CoreText.mm

+        // Ignore return value, we only want the lineCount for now
+        textLayout->GetLineMetrics(nullptr, 0, &lineCount);
+
+        std::vector<DWRITE_LINE_METRICS> metrics(lineCount);


::vector<DWRITE_LINE_METRICS> metrics(lineCount); [](start = 11, length = 49)

since this is about performance, use array rather than a vector here (you will have a slight gain) #Closed

hmm I guess you might not know the size? nvm

In reply to: 93508769 [](ancestors = 93508769)

msft-Jeyaram · 2016-12-21T20:00:49Z

Frameworks/UIKit/NSString+UIKitAdditions.mm

 }

 /**
 @Status Interoperable
 */
- (CGSize)drawAtPoint:(CGPoint)pt withFont:(UIFont*)font {
-    CGSize fontExtent;
+- (CGSize)drawInRect:(CGRect)rect withFont:(UIFont*)font lineBreakMode:(UILineBreakMode)lineBreakMode alignment:(UITextAlignment)alignment {


font [](start = 52, length = 4)

does the validity of the font matter? #Closed

Drawing with nil font just uses default value. Actually didn't mean to get rid of that in this change, but I think it makes sense to do. @ms-jihua any thoughts? #Closed

uses default value as in, WinObjC currently uses a default value if a nil font is passed to DWrite, or the reference platform uses a default value? if the former, how?

my recommendation is to verify whether the reference platform uses a default font if nil is passed - if so, set font = [UIFont defaultFont] instead of putting nil in a dict

In reply to: 93509601 [](ancestors = 93509601)

msft-Jeyaram · 2016-12-21T20:05:35Z

Frameworks/UIKit/NSString+UIKitAdditions.mm

+    CGContextRef context = UIGraphicsGetCurrentContext();
+    if (rect.size.width == 0) {
+        rect.size.width = std::numeric_limits<CGFloat>::max();
+    }


is the intended behavior is to extend the region, specifically if one of the constraints are 0? #Closed

some comments on the extension of area would be good to have.

In reply to: 93510034 [](ancestors = 93510034)

msft-Jeyaram · 2016-12-21T20:14:00Z

Frameworks/CoreText/DWriteWrapper_CoreText.mm

+
+    if (!string || range.length <= 0L) {
+        return ret;
+    }


"If the length portion of the range is set to 0, then the framesetter continues to add lines until it runs out of text or space."
This does not seem to match that behaviour #Closed

wut? #Closed

msft-Jeyaram · 2016-12-21T20:15:44Z

Frameworks/CoreText/CTFramesetter.mm

+    CFAttributedStringRef string =
+        static_cast<CFAttributedStringRef>(static_cast<_CTFramesetter*>(framesetter)->_typesetter->_attributedString.get());
+
+    _DWriteGetSize(string, stringRange, constraints, fitRange);


should be returning this value :) #Closed

msft-Jeyaram · 2016-12-21T20:19:28Z

Frameworks/UIKit/NSString+UIKitAdditions.mm


-    CGRect rct;
+    CTParagraphStyleSetting styles[2];
+    CTTextAlignment align = _NSTextAlignmentToCTTextAlignment(alignment);


_NSTextAlignmentToCTTextAlignment [](start = 28, length = 33)

this seem to be a a public function, why is it private?
move the implementation to the actual function #Closed

msft-Jeyaram · 2016-12-21T20:22:24Z

Frameworks/UIKit/NSString+UIKitAdditions.mm

-    rct.origin.y = pt.y;
-    rct.size.width = 0;
-    rct.size.height = 0;
+    CTLineBreakMode breakMode = static_cast<CTLineBreakMode>(lineBreakMode);


lineBreakMode [](start = 61, length = 13)

some of the below functions indicate this is not fully supported, update? #Closed

msft-Jeyaram · 2016-12-21T20:25:47Z

Frameworks/UIKit/NSString+UIKitAdditions.mm

+    NSAttributedString* attr =
+        [[[NSAttributedString alloc] initWithString:self
+                                         attributes:@{
+                                             (NSString*)kCTForegroundColorFromContextAttributeName : (id)kCFBooleanTrue,


(NSString*) [](start = 45, length = 11)

switching to static cast is going to really obscure readability.
I guess we can leave it as be #Closed

msft-Jeyaram · 2016-12-21T20:26:55Z

Frameworks/UIKit/NSString+UIKitAdditions.mm

+        [[[NSAttributedString alloc] initWithString:self
+                                         attributes:@{
+                                             (NSString*)kCTForegroundColorFromContextAttributeName : (id)kCFBooleanTrue,
+                                             (NSString*)kCTParagraphStyleAttributeName : (id)CTParagraphStyleCreate(styles, 2),


hmmm given it's a compile time allocated array, can deduce the count from the array? #Closed

msft-Jeyaram · 2016-12-21T20:32:22Z

Frameworks/UIKit/NSString+UIKitAdditions.mm


-    drawString(font, UIGraphicsGetCurrentContext(), self, rct, UILineBreakModeClip, UITextAlignmentLeft, &fontExtent);
+    return _CTFrameGetSize(frame.get());


returning a zero size here if something fails above.
Does that match the intended behavior? #Closed

msft-Jeyaram · 2016-12-21T20:34:58Z

Frameworks/UIKit/NSString+UIKitAdditions.mm

+    woc::unique_cf<CTFramesetterRef> framesetter{ CTFramesetterCreateWithAttributedString(static_cast<CFAttributedStringRef>(attr)) };
+
+    woc::unique_cf<CGPathRef> path{ CGPathCreateWithRect(rect, nullptr) };
+    woc::unique_cf<CTFrameRef> frame{ CTFramesetterCreateFrame(framesetter.get(), {}, path.get(), nullptr) };


I would perfer you catch the failures of creation here rather than adding more functions calls, attempt to allocate a vector of size 0 and then returning the size. #Closed

also this ends up propagating the errors and makes it harder to catch later on.

In reply to: 93514860 [](ancestors = 93514860)

+1

In reply to: 93514919 [](ancestors = 93514919,93514860)

msft-Jeyaram · 2016-12-21T20:49:51Z

Frameworks/UIKit/NSString+UIKitAdditions.mm

 }

 /**
 @Status Interoperable
 */
- (CGSize)drawAtPoint:(CGPoint)pt withFont:(UIFont*)font {
-    CGSize fontExtent;
+- (CGSize)drawInRect:(CGRect)rect withFont:(UIFont*)font lineBreakMode:(UILineBreakMode)lineBreakMode alignment:(UITextAlignment)alignment {


how are these being verified? #Closed

Just visually using test apps, we don't have any unit/drawing tests for UI kit

msft-Jeyaram · 2016-12-21T20:52:59Z

Frameworks/UIKit/NSString+UIKitAdditions.mm

+                                             (NSString*)kCTFontAttributeName : font
+                                         }] autorelease];
+
+    woc::unique_cf<CTFramesetterRef> framesetter{ CTFramesetterCreateWithAttributedString(static_cast<CFAttributedStringRef>(attr)) };


framesetter [](start = 37, length = 11)

RETURN_RESULT_IF_NULL(framesetter,CGSizeZero);

msft-Jeyaram · 2016-12-21T20:53:06Z

Frameworks/UIKit/NSString+UIKitAdditions.mm

+
+    woc::unique_cf<CTFramesetterRef> framesetter{ CTFramesetterCreateWithAttributedString(static_cast<CFAttributedStringRef>(attr)) };
+
+    woc::unique_cf<CGPathRef> path{ CGPathCreateWithRect(rect, nullptr) };


{ [](start = 34, length = 1)

RETURN_RESULT_IF_NULL(path,CGSizeZero);

msft-Jeyaram · 2016-12-21T20:53:18Z

Frameworks/UIKit/NSString+UIKitAdditions.mm

+    woc::unique_cf<CTFramesetterRef> framesetter{ CTFramesetterCreateWithAttributedString(static_cast<CFAttributedStringRef>(attr)) };
+
+    woc::unique_cf<CGPathRef> path{ CGPathCreateWithRect(rect, nullptr) };
+    woc::unique_cf<CTFrameRef> frame{ CTFramesetterCreateFrame(framesetter.get(), {}, path.get(), nullptr) };


frame [](start = 31, length = 5)

RETURN_RESULT_IF_NULL(frame,CGSizeZero);
:) #Closed

msft-Jeyaram

🍰

ms-jihua · 2016-12-27T19:10:23Z

#endif

unrelated, but can you see if we can ditch these? UIKIT_EXPORT already has 'extern "C"' when __cplusplus is defined #Closed

Refers to: include/UIKit/UIGraphics.h:40 in c3bbc8f. [](commit_id = c3bbc8f, deletion_comment = False)

ms-jihua · 2016-12-27T19:25:11Z

any idea of the savings? rough measurements are fine

ms-jihua · 2016-12-27T19:30:09Z

Frameworks/CoreText/DWriteWrapper_CoreText.mm

+static CGSize _DWriteGetSize(CFAttributedStringRef string, CFRange range, CGSize maxSize, CFRange* fitRange) {
+    CGSize ret = CGSizeZero;
+    if (range.length == 0L) {
+        range.length = CFAttributedStringGetLength(string);


I'm guessing @msft-Jeyaram missed this part up here wrt the comment below? (thinking that probably at least one of these two deserves a callout in a comment) #Closed

ms-jihua · 2016-12-27T19:31:43Z

Frameworks/CoreText/DWriteWrapper_CoreText.h

@@ -51,6 +51,7 @@ struct _DWriteGlyphRunDetails {

 bool _CloneDWriteGlyphRun(_In_ DWRITE_GLYPH_RUN const* src, _Outptr_ DWRITE_GLYPH_RUN* dest);

+CGSize _DWriteGetSize(CFAttributedStringRef string, CFRange range, CGSize maxSize, CFRange* fitRange);


_DWriteGetSize [](start = 7, length = 14)

nit: would prefer more descriptive name #Closed

ms-jihua · 2016-12-27T19:34:17Z

Frameworks/CoreText/DWriteWrapper_CoreText.mm

+
+        float totalHeight = 0;
+        CFIndex endPos = range.location;
+        for (size_t i = 0; i < lineCount; ++i) {


nit: prefer enhanced for

extra nit: const auto&

In reply to: 93963412 [](ancestors = 93963412)

ms-jihua · 2016-12-27T19:57:25Z

Frameworks/UIKit/NSString+UIKitAdditions.mm

- (CGSize)drawAtPoint:(CGPoint)pt withFont:(UIFont*)font {
-    CGSize fontExtent;
+- (CGSize)drawInRect:(CGRect)rect withFont:(UIFont*)font lineBreakMode:(UILineBreakMode)lineBreakMode alignment:(UITextAlignment)alignment {
+    CGContextRef context = UIGraphicsGetCurrentContext();


create this lower, this isn't needed for a while #Closed

ms-jihua · 2016-12-27T19:59:56Z

rajsesh · 2017-01-04T01:15:43Z

#endif

Or include the new definition before the ending #ifdef to be consistent.

In reply to: 269369006 [](ancestors = 269369006)

Refers to: include/UIKit/UIGraphics.h:40 in c3bbc8f. [](commit_id = c3bbc8f, deletion_comment = False)

aballway · 2017-01-09T17:13:11Z

It cuts down frame measurement time a little more than half, saving about 25% of CPU time drawing in the XAML custom text editor.

In reply to: 269371016 [](ancestors = 269371016)

aballway · 2017-01-10T16:15:54Z

Ping! Please update reviews

Improve perf for CTFramesetterSuggestFrameSizeWithConstraints

1191d4a

aballway requested review from rajsesh and ms-jihua December 21, 2016 19:05

msftclas added the cla-not-required label Dec 21, 2016

msft-Jeyaram reviewed Dec 21, 2016

View reviewed changes

Clean up NSString+UIKitAdditions, CR feedback

914dc37

msft-Jeyaram reviewed Dec 21, 2016

View reviewed changes

How did that compile?!?

550dd01

msft-Jeyaram reviewed Dec 21, 2016

View reviewed changes

msft-Jeyaram suggested changes Dec 21, 2016

View reviewed changes

Address CR feedback

c3bbc8f

ms-jihua reviewed Dec 27, 2016

View reviewed changes

ms-jihua mentioned this pull request Jan 3, 2017

Investigate perf issue with _DWriteGetFrame() #1621

Closed

aballway and others added 2 commits January 9, 2017 09:16

CR feedback

16d3b26

Update missed status for drawInRect

5f9610b

ms-jihua approved these changes Jan 10, 2017

View reviewed changes

msft-Jeyaram approved these changes Jan 10, 2017

View reviewed changes

Fix build break

df0f4f7

aballway merged commit caf13e1 into microsoft:develop Jan 12, 2017


		drawString(font, UIGraphicsGetCurrentContext(), self, rct, UILineBreakModeClip, UITextAlignmentLeft, &fontExtent);
		return _CTFrameGetSize(frame.get());


		woc::unique_cf<CTFramesetterRef> framesetter{ CTFramesetterCreateWithAttributedString(static_cast<CFAttributedStringRef>(attr)) };

		woc::unique_cf<CGPathRef> path{ CGPathCreateWithRect(rect, nullptr) };

		@@ -51,6 +51,7 @@ struct _DWriteGlyphRunDetails {

		bool _CloneDWriteGlyphRun(_In_ DWRITE_GLYPH_RUN const* src, _Outptr_ DWRITE_GLYPH_RUN* dest);

		CGSize _DWriteGetSize(CFAttributedStringRef string, CFRange range, CGSize maxSize, CFRange* fitRange);

Improve perf for CTFramesetterSuggestFrameSizeWithConstraints #1603

Improve perf for CTFramesetterSuggestFrameSizeWithConstraints #1603

Conversation

aballway commented Dec 21, 2016 • edited Loading

msft-Jeyaram Dec 21, 2016 • edited by aballway Loading

Choose a reason for hiding this comment

msft-Jeyaram Dec 21, 2016 • edited by aballway Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msft-Jeyaram Dec 21, 2016 • edited by aballway Loading

Choose a reason for hiding this comment

aballway Dec 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msft-Jeyaram Dec 21, 2016 • edited by aballway Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msft-Jeyaram Dec 21, 2016 • edited by aballway Loading

Choose a reason for hiding this comment

aballway Dec 21, 2016 • edited by ms-jihua Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msft-Jeyaram Dec 21, 2016 • edited by aballway Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msft-Jeyaram Dec 21, 2016 • edited by aballway Loading

Choose a reason for hiding this comment

aballway Dec 22, 2016 • edited Loading

Choose a reason for hiding this comment

msft-Jeyaram Dec 21, 2016 • edited by aballway Loading

Choose a reason for hiding this comment

msft-Jeyaram Dec 21, 2016 • edited by aballway Loading

Choose a reason for hiding this comment

msft-Jeyaram Dec 21, 2016 • edited by aballway Loading

Choose a reason for hiding this comment

msft-Jeyaram Dec 21, 2016 • edited by aballway Loading

Choose a reason for hiding this comment

msft-Jeyaram Dec 21, 2016 • edited by aballway Loading

Choose a reason for hiding this comment

msft-Jeyaram Dec 21, 2016 • edited by aballway Loading

Choose a reason for hiding this comment

msft-Jeyaram Dec 21, 2016 • edited by aballway Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msft-Jeyaram Dec 21, 2016 • edited by aballway Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msft-Jeyaram Dec 21, 2016 • edited by aballway Loading

Choose a reason for hiding this comment

msft-Jeyaram left a comment

Choose a reason for hiding this comment

ms-jihua commented Dec 27, 2016 • edited Loading

ms-jihua commented Dec 27, 2016

ms-jihua Dec 27, 2016 • edited by aballway Loading

Choose a reason for hiding this comment

ms-jihua Dec 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ms-jihua Dec 27, 2016 • edited Loading

Choose a reason for hiding this comment

ms-jihua commented Dec 27, 2016 • edited Loading

rajsesh commented Jan 4, 2017

aballway commented Jan 9, 2017

aballway commented Jan 10, 2017

aballway commented Dec 21, 2016 •

edited

Loading

msft-Jeyaram Dec 21, 2016 •

edited by aballway

Loading

msft-Jeyaram Dec 21, 2016 •

edited by aballway

Loading

msft-Jeyaram Dec 21, 2016 •

edited by aballway

Loading

aballway Dec 21, 2016 •

edited

Loading

msft-Jeyaram Dec 21, 2016 •

edited by aballway

Loading

msft-Jeyaram Dec 21, 2016 •

edited by aballway

Loading

aballway Dec 21, 2016 •

edited by ms-jihua

Loading

msft-Jeyaram Dec 21, 2016 •

edited by aballway

Loading

msft-Jeyaram Dec 21, 2016 •

edited by aballway

Loading

aballway Dec 22, 2016 •

edited

Loading

msft-Jeyaram Dec 21, 2016 •

edited by aballway

Loading

msft-Jeyaram Dec 21, 2016 •

edited by aballway

Loading

msft-Jeyaram Dec 21, 2016 •

edited by aballway

Loading

msft-Jeyaram Dec 21, 2016 •

edited by aballway

Loading

msft-Jeyaram Dec 21, 2016 •

edited by aballway

Loading

msft-Jeyaram Dec 21, 2016 •

edited by aballway

Loading

msft-Jeyaram Dec 21, 2016 •

edited by aballway

Loading

msft-Jeyaram Dec 21, 2016 •

edited by aballway

Loading

msft-Jeyaram Dec 21, 2016 •

edited by aballway

Loading

ms-jihua commented Dec 27, 2016 •

edited

Loading

ms-jihua Dec 27, 2016 •

edited by aballway

Loading

ms-jihua Dec 27, 2016 •

edited

Loading

ms-jihua Dec 27, 2016 •

edited

Loading

ms-jihua commented Dec 27, 2016 •

edited

Loading