Re-Implementation of reverse string via CFString #2479

msft-Jeyaram · 2017-04-10T22:41:05Z

Implement _reverseString via CoreFoundation and _wcsrev
Remove _reverseString from Foundation (only CoreText uses it)
CTline _getTruncatedStringFromSourceLine should use CF*based reverse string
Remove redundant tests

fixes #2478

DHowett-MSFT · 2017-04-10T22:41:23Z

nit: reword to be imperative mood #Pending

msft-Jeyaram · 2017-04-10T22:41:37Z

Getting some bench mark for old vs new #Resolved

rajsesh · 2017-04-10T22:44:23Z

Frameworks/CoreFoundationAdditions/CFStringHelpers.mm

+
+    CFMutableStringRef result = CFStringCreateMutable(kCFAllocatorDefault, 0);
+
+    while (index > 0) {


This is going to suck when it comes to perf. #Resolved

might as well use buffer indexing, given it's somewhat "safe"

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

DHowett-MSFT

I like the use of unichars and individual character access, because it's safe for multi-byte encodings... but we should be cognizant of allocations!

DHowett-MSFT · 2017-04-10T22:41:57Z

Frameworks/CoreFoundationAdditions/CFStringHelpers.mm

+    CFIndex index = CFStringGetLength(string);
+
+    if (index < 2) {
+        return CFStringCreateCopy(kCFAllocatorDefault, string);


this could also be CFRetain #ByDesign

If the input is a mutable string, retain might not be a good idea/ #ByDesign

Good point! #ByDesign

DHowett-MSFT · 2017-04-10T22:43:24Z

Frameworks/CoreFoundationAdditions/CFStringHelpers.mm

+        return CFStringCreateCopy(kCFAllocatorDefault, string);
+    }
+
+    CFMutableStringRef result = CFStringCreateMutable(kCFAllocatorDefault, 0);


If you create the new string with a capacity hint from the original string, you can avoid the inevitable reallocations #Resolved

DHowett-MSFT · 2017-04-10T22:44:12Z

Frameworks/CoreFoundationAdditions/CFStringHelpers.mm

+
+    CFMutableStringRef result = CFStringCreateMutable(kCFAllocatorDefault, 0);
+
+    while (index > 0) {


it may be cheaper to create a unichar buffer and pass that off to CFStringCreateWithCharacters -- you get the added benefit of not creating a mutable string

It should be somewhat "safe" to use utf-16 codepoints or unichars; it's definitely safer than utf-8 #Resolved

that's what i was going with first, it might be more sensible given that only CT uses it.
Hmm i might as well move it in there, but this kind of makes it more util based. #Resolved

YEP utf-16 is safer than utf-8 :)

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

DHowett-MSFT · 2017-04-10T22:45:43Z

Also, I wonder how it works with composing diacritics. It might be reasonable to normalize the string first... #Resolved

rajsesh · 2017-04-10T22:47:46Z

Frameworks/CoreFoundationAdditions/CFStringHelpers.mm

+#import <CFInternal.h>
+#import "CFFoundationInternal.h"
+
+CFStringRef _CFStringCreateReversedString(CFStringRef string) {


Given that there is exactly one place where this is used (and was used in the past), I don't like that we are doing this. Why not have the helper in CT and just use it there? #ByDesign

That's what my initial plan was, but since this seems very common and could be utilized by others, it would make a good addition. Also we already have some tests that can be tested independently. #Resolved

Lets not do this. the _reverseString was added because some other code was using it. Given that there is only one instance where _reverseString was used, there is no need to go overboard here.

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

also, what is wrong with good old fashioned strrev variants?

In reply to: 110782273 [](ancestors = 110782273,110781327)

_strrev + UTF-16 encoding should do the trick then.

In reply to: 110782324 [](ancestors = 110782324,110782273,110781327)

rajsesh

Lets not do this at all.

msft-Jeyaram · 2017-04-10T23:10:46Z

@rajsesh-msft still need reverse in CFString backed by strrev, that is contained in CoreText.
Updating the CR #Resolved

msft-Jeyaram · 2017-04-11T02:02:19Z

Frameworks/CoreText/CTUtilities.mm

@@ -15,7 +15,10 @@
 //******************************************************************************

 #import <CoreText/CTUtilities.h>
+#import <Starboard/SmartTypes.h>


removed #Resolved

ms-jihua · 2017-04-11T16:57:27Z

Frameworks/CoreText/CTLine.mm

@@ -188,8 +187,9 @@ CTLineRef CTLineCreateTruncatedLine(CTLineRef sourceLine, double width, CTLineTr
        CFDictionaryRef attribs = CTRunGetAttributes(currentRun);

        if (truncationType == kCTLineTruncationStart) {
-            NSString* reverse = [runString _reverseString];
-            NSAttributedString* string = [[NSAttributedString alloc] initWithString:reverse attributes:(NSDictionary*)attribs];
+            auto reverse = woc::MakeStrongCF<CFStringRef>(_CTCreateReversedString(static_cast<CFStringRef>(runString)));


if there's just the one usage of it, can it be a file-scope function instead? #Resolved

msft-Jeyaram · 2017-04-11T17:26:40Z

Frameworks/CoreText/CTUtilities.mm

+    }
+
+    // Returns the number (in terms of UTF-16 code pairs) of Unicode characters in a string.
+    CFIndex length = CFStringGetLength(string);


using CFStringGetBytes to be safer
#Resolved

msft-Jeyaram · 2017-04-11T19:05:09Z

@DHowett-MSFT @ms-jihua please take a look

DHowett-MSFT · 2017-04-11T19:05:59Z

Frameworks/CoreText/CTLine.mm

 #import "CoreTextInternal.h"
 #import "CGContextInternal.h"
 #import "DWriteWrapper_CoreText.h"
 #import <CoreText/CTTypesetter.h>

 #import <algorithm>
 #import <numeric>
+#import <wchar.h>


nit: prefer <cwchar> #Resolved

DHowett-MSFT · 2017-04-11T19:07:09Z

Frameworks/CoreText/CTLine.mm

+        return CFStringCreateCopy(kCFAllocatorDefault, string);
+    }
+
+    std::vector<UniChar> characters((usedBufLen / (sizeof(UniChar))) + 1, '\0');


If you use unique_iw instead of std::vector, you can use CFStringCreateWithBytesNoCopy and detach it instead of copying the buffer. #Resolved

DHowett-MSFT · 2017-04-11T19:07:44Z

Frameworks/CoreText/CTLine.mm

-            NSAttributedString* string = [[NSAttributedString alloc] initWithString:reverse attributes:(NSDictionary*)attribs];
+            auto reverse = woc::MakeStrongCF<CFStringRef>(_CTCreateReversedString(static_cast<CFStringRef>(runString)));
+            NSAttributedString* string =
+                [[NSAttributedString alloc] initWithString:static_cast<NSString*>(reverse.get()) attributes:(NSDictionary*)attribs];


??? can't we use CFAttributedStringCreate? #ByDesign

I already have changes for all NSAttributed, this is strictly for the reverse. #ByDesign

DHowett-MSFT

Sorry, I chose the wrong option.

ms-jihua · 2017-04-11T20:13:46Z

Frameworks/CoreText/CTLine.mm

+
+    CFIndex length = CFStringGetLength(string);
+    CFIndex usedBufLen;
+    CFStringGetBytes(string, CFRangeMake(0, length), kCFStringEncodingUTF16, 0, false, nullptr, length, &usedBufLen);


Seems kind of expensive for this purpose (unless CFStringGetBytes has a fastpath here?)

I wonder if CFStringGetMaximumSizeForEncoding() would be faster/would still work? #ByDesign

Yes it does take a faster way than if you provide a buffer.
Further improvements can be addressed by a bug to improve core text. The CR is mainly to remove Foundation dependency and what we have now is better than the earlier Foundation/copy based implementation.
#ByDesign

aballway · 2017-04-11T20:38:21Z

Frameworks/CoreText/CTLine.mm

 #import <vector>

+static CFStringRef __CTCreateReversedString(CFStringRef string) {


how is this change being tested? #Resolved

UnitTests

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

DHowett-MSFT · 2017-04-13T00:09:29Z

Frameworks/CoreText/CTLine.mm

+
+    CFIndex bufLen = usedBufLen + 1 * sizeof(UniChar);
+    woc::unique_iw<UniChar> characters(static_cast<UniChar*>(IwMalloc(bufLen)));
+    memset(characters.get(), '\0', bufLen);


c'mon, you only need to set the last character in the buffer

🤣 that was coming 😄

DHowett-MSFT · 2017-04-13T00:12:11Z

Frameworks/CoreText/CTLine.mm

+                                         bufLen,
+                                         kCFStringEncodingUTF16,
+                                         false,
+                                         nullptr);


you will need to provide an allocator to delete this string; look at the implementation of initWithBytesNoCopy:freeWhenDone: for NSString, expecting freeWhenDone=YES

Already passing in the nullptr for deallocation.
Switched to use characters, which does it for you.

msft-Jeyaram · 2017-04-19T22:53:16Z

@DHowett-MSFT please take a look

- Implemented via _wcsrev - Reduced string copies - Implemented using CoreFoundation - Removed usage of Foundation associated with reverse string. Fixes microsoft#2478

aballway · 2017-04-20T16:14:21Z

Frameworks/CoreText/CTLine.mm

+    CFIndex usedBufLen;
+    CFStringGetBytes(string, CFRangeMake(0, length), kCFStringEncodingUTF16, 0, false, nullptr, length, &usedBufLen);
+
+    if (length < 2) {


nit: move this above CFStringGetBytes call

aballway · 2017-04-20T16:16:31Z

Frameworks/include/CFFoundationInternal.h

@@ -1,6 +1,6 @@
 //******************************************************************************
 //
-// Copyright (c) 2015 Microsoft Corporation. All rights reserved.
+// Copyright (c) Microsoft. All rights reserved.


These clean-ups, though nice, seem unrelated to this PR. Just revert these files?

msft-Jeyaram requested review from aballway, DHowett-MSFT, rajsesh and ms-jihua April 10, 2017 22:41

msftclas added the cla-not-required label Apr 10, 2017

rajsesh reviewed Apr 10, 2017

View reviewed changes

DHowett-MSFT suggested changes Apr 10, 2017

View reviewed changes

rajsesh reviewed Apr 10, 2017

View reviewed changes

rajsesh suggested changes Apr 10, 2017

View reviewed changes

msft-Jeyaram force-pushed the RevMove branch from 85fedc1 to b1dd6c8 Compare April 11, 2017 01:40

msft-Jeyaram changed the title ~~Re-Implementation of reverse string safely via CFString~~ Re-Implementation of reverse string via CFString Apr 11, 2017

msft-Jeyaram commented Apr 11, 2017

View reviewed changes

ms-jihua reviewed Apr 11, 2017

View reviewed changes

msft-Jeyaram commented Apr 11, 2017

View reviewed changes

DHowett-MSFT approved these changes Apr 11, 2017

View reviewed changes

DHowett-MSFT suggested changes Apr 11, 2017

View reviewed changes

ms-jihua reviewed Apr 11, 2017

View reviewed changes

aballway reviewed Apr 11, 2017

View reviewed changes

rajsesh approved these changes Apr 11, 2017

View reviewed changes

ms-jihua approved these changes Apr 11, 2017

View reviewed changes

DHowett-MSFT suggested changes Apr 13, 2017

View reviewed changes

msft-Jeyaram force-pushed the RevMove branch from cd6954c to 801b71b Compare April 19, 2017 22:52

DHowett-MSFT approved these changes Apr 19, 2017

View reviewed changes

Updated proper copyright information

4e72b6b

msft-Jeyaram added 3 commits April 19, 2017 17:00

Implementation of reverse string.

3bf160d

- Implemented via _wcsrev - Reduced string copies - Implemented using CoreFoundation - Removed usage of Foundation associated with reverse string. Fixes microsoft#2478

Remove the Foundation copy based implementation of string reversal

c98940e

Delete redundant string reversal tests.

afcfbad

msft-Jeyaram force-pushed the RevMove branch from 801b71b to afcfbad Compare April 20, 2017 00:25

aballway reviewed Apr 20, 2017

View reviewed changes

msft-Jeyaram merged commit a8672e9 into microsoft:develop Apr 20, 2017

msft-Jeyaram mentioned this pull request Apr 27, 2017

Break CoreGraphics and CoreText's dependencies on Foundation + UIKit #2026

Closed


		CFMutableStringRef result = CFStringCreateMutable(kCFAllocatorDefault, 0);

		while (index > 0) {

		#import <vector>

		static CFStringRef __CTCreateReversedString(CFStringRef string) {

Re-Implementation of reverse string via CFString #2479

Re-Implementation of reverse string via CFString #2479

Conversation

msft-Jeyaram commented Apr 10, 2017 • edited Loading

DHowett-MSFT commented Apr 10, 2017 • edited by msft-Jeyaram Loading

msft-Jeyaram commented Apr 10, 2017 • edited Loading

rajsesh Apr 10, 2017 • edited by msft-Jeyaram Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett-MSFT left a comment

Choose a reason for hiding this comment

DHowett-MSFT Apr 10, 2017 • edited by msft-Jeyaram Loading

Choose a reason for hiding this comment

msft-Jeyaram Apr 10, 2017 • edited Loading

Choose a reason for hiding this comment

DHowett-MSFT Apr 10, 2017 • edited by msft-Jeyaram Loading

Choose a reason for hiding this comment

DHowett-MSFT Apr 10, 2017 • edited by msft-Jeyaram Loading

Choose a reason for hiding this comment

DHowett-MSFT Apr 10, 2017 • edited by msft-Jeyaram Loading

Choose a reason for hiding this comment

msft-Jeyaram Apr 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett-MSFT commented Apr 10, 2017 • edited by msft-Jeyaram Loading

rajsesh Apr 10, 2017 • edited by msft-Jeyaram Loading

Choose a reason for hiding this comment

msft-Jeyaram Apr 10, 2017 • 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

rajsesh left a comment

Choose a reason for hiding this comment

msft-Jeyaram commented Apr 10, 2017 • edited Loading

msft-Jeyaram Apr 11, 2017 • edited Loading

Choose a reason for hiding this comment

ms-jihua Apr 11, 2017 • edited by msft-Jeyaram Loading

Choose a reason for hiding this comment

msft-Jeyaram Apr 11, 2017 • edited Loading

Choose a reason for hiding this comment

msft-Jeyaram commented Apr 11, 2017

DHowett-MSFT Apr 11, 2017 • edited by msft-Jeyaram Loading

Choose a reason for hiding this comment

DHowett-MSFT Apr 11, 2017 • edited by msft-Jeyaram Loading

Choose a reason for hiding this comment

DHowett-MSFT Apr 11, 2017 • edited by msft-Jeyaram Loading

Choose a reason for hiding this comment

msft-Jeyaram Apr 11, 2017 • edited Loading

Choose a reason for hiding this comment

DHowett-MSFT left a comment

Choose a reason for hiding this comment

ms-jihua Apr 11, 2017 • edited by msft-Jeyaram Loading

Choose a reason for hiding this comment

msft-Jeyaram Apr 11, 2017 • edited Loading

Choose a reason for hiding this comment

aballway Apr 11, 2017 • edited by msft-Jeyaram 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

Choose a reason for hiding this comment

msft-Jeyaram commented Apr 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msft-Jeyaram commented Apr 10, 2017 •

edited

Loading

DHowett-MSFT commented Apr 10, 2017 •

edited by msft-Jeyaram

Loading

msft-Jeyaram commented Apr 10, 2017 •

edited

Loading

rajsesh Apr 10, 2017 •

edited by msft-Jeyaram

Loading

DHowett-MSFT Apr 10, 2017 •

edited by msft-Jeyaram

Loading

msft-Jeyaram Apr 10, 2017 •

edited

Loading

DHowett-MSFT Apr 10, 2017 •

edited by msft-Jeyaram

Loading

DHowett-MSFT Apr 10, 2017 •

edited by msft-Jeyaram

Loading

DHowett-MSFT Apr 10, 2017 •

edited by msft-Jeyaram

Loading

msft-Jeyaram Apr 10, 2017 •

edited

Loading

DHowett-MSFT commented Apr 10, 2017 •

edited by msft-Jeyaram

Loading

rajsesh Apr 10, 2017 •

edited by msft-Jeyaram

Loading

msft-Jeyaram Apr 10, 2017 •

edited

Loading

msft-Jeyaram commented Apr 10, 2017 •

edited

Loading

msft-Jeyaram Apr 11, 2017 •

edited

Loading

ms-jihua Apr 11, 2017 •

edited by msft-Jeyaram

Loading

msft-Jeyaram Apr 11, 2017 •

edited

Loading

DHowett-MSFT Apr 11, 2017 •

edited by msft-Jeyaram

Loading

DHowett-MSFT Apr 11, 2017 •

edited by msft-Jeyaram

Loading

DHowett-MSFT Apr 11, 2017 •

edited by msft-Jeyaram

Loading

msft-Jeyaram Apr 11, 2017 •

edited

Loading

ms-jihua Apr 11, 2017 •

edited by msft-Jeyaram

Loading

msft-Jeyaram Apr 11, 2017 •

edited

Loading

aballway Apr 11, 2017 •

edited by msft-Jeyaram

Loading