Improve perf in KVO and NSString #2323

ms-jihua · 2017-03-24T21:40:24Z

Fixed a bug in CFStringGetCStringPtr where it called _fastCStringContents with an NSString encoding,
but _fastCStringContents expected a CFString encoding. As a result, CFStringGetCStringPtr now actually returns something for most strings.
- Similarly, [NSString UTF8String] on a literal now returns a fast pointer rather than an autorelease buffer.
Changed [NSString isEqualToString:], [NSConstantString hash], and [NSConstantString isEqualToString:] to rely on faster functions
- Made a similar change to speed up a local helper (sorry @aballway i misled you)
Changed _NSKVCSplitKeypath, [Class keyPathsForValuesAffectingKey:] to use C-string parsing
- added a private helper for keyPathsForValuesAffectingKey: to not construct the empty set, for internal usage
Changed NSKVOKeypathObserver to store an observer as assign instead of as weak.
This makes us theoretically less safe if an observer deallocs without removing itself as an observer.
However, the observed object already threw once it reached its dealloc and still had observers,
and the above case is considered incorrect anyway,
so it only allowed us to fail quietly in the case where the observed object has a much longer lifetime, which is of questionable value.
Opting in favor of assign here, as it provided a substantial (-~40%) decrease in time it takes to addObserver:
Changed _dispatchWillChange to just take an NSMutableDictionary instead of doing an unnecessary mutableCopy
Minor change to setObservationInfo: to be atomic on the associated object table rather than the object itself
Added more benchmark tests for KVO, String scenarios

Fixes #2040

ms-jihua · 2017-03-24T21:47:01Z

Perf numbers, with some numbers from before #2240 for comparison

Improvements across the board, but we're still slower than the old NSOperationQueue on adding operations (though not executing them). (Sorry for misleading you on this @rajsesh-msft, I forgot I switched hardware recently and would get lower numbers). The cost of addOperation: is currently about 2/3 KVO addObserver: and 1/3 libdispatch.

The biggest cost in KVO as it currently stands is construction of objc objects - NSKVOObservationInfo et al seem like they could easily be lighter-weight structs, but this would be a fairly heavy change that I didn't want to get into while @DHowett-MSFT was out. If we want to further pursue KVO perf, I can discuss the issue with him once he returns.

aballway · 2017-03-24T22:12:07Z

Frameworks/Foundation/NSKVOSupport.mm

+// Private helper that backs the default implementation of keyPathsForValuesAffectingValueForKey:
+// Returns nil instead of constructing an empty set if no keyPaths are found
+// Also used internally as a minor optimization, to avoid constructing an empty set when it is not needed
+static NSSet* _keyPathsForValuesAffectingValueForKey(Class self, NSString* key) {


nit: rename self to something that's not a keyword.

I think we have other instances of 'self' being what it would normally be, were this an objc function. Wontfixing this.

aballway · 2017-03-24T22:18:54Z

Frameworks/CoreText/DWriteWrapper_CTFont.mm

-    CFIndex leftLength = CFStringGetLength(left);
-    return leftLength == CFStringGetLength(right) &&
-           CFStringFindWithOptions(left, right, { 0, leftLength }, kCFCompareCaseInsensitive, nullptr);
+    return CFStringCompare(left, right, kCFCompareCaseInsensitive) == kCFCompareEqualTo;


aballway · 2017-03-24T22:22:08Z

Frameworks/Foundation/NSKVOSupport.mm

-        }
-    }
-    return [NSSet set];
+    NSSet* ret = _keyPathsForValuesAffectingValueForKey(self, key);


_keyPathsForValuesAffectingValueForKey takes a Class but this is an NSObject?

This is a + function, so self is a Class.

rajsesh · 2017-03-24T22:32:08Z

build/Tests/Benchmark/Framework.Benchmark.vcxproj

@@ -221,7 +221,9 @@
  </ItemGroup>
  <ItemGroup>
    <ClangCompile Include="$(StarboardBasePath)\tests\Benchmark\BenchmarkSampleTests.mm" />
+    <ClangCompile Include="$(StarboardBasePath)\tests\Benchmark\KVOBenchmarkTests.mm" />


There is also a tests\testapps\kvoperf, which is probably obsolete now if we move all those tests here.

I forgot about that - I don't think I'm comfortable removing it while @DHowett-MSFT is out, but I'll take a look and see if anything can be copied from there to here.

That app is not used at all (it is a manual tests). When it was written, we didn't have the benchmark tests. It should be safe to remove once all the pieces of it are salvaged.

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

It's more or less the same scenario as the ObserveChange benchmark, but also measures the amount of time spent in the actual notification part, which the benchmark framework doesn't really have a mechanism for.

(as opposed to ObserveChange, the app is also a scenario with just one observer - do we want to add that to benchmarks, or do we consider ObserveChange's many-observer scenario mostly representative of health?)

In any case, before/after:

The multiple observer scenario is important. We do see some improvement there so thats good to see.

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

rajsesh · 2017-03-24T22:35:44Z

Frameworks/CoreText/DWriteWrapper_CTFont.mm

@@ -43,9 +43,7 @@
 * Helper function that efficiently compares two CFStringRef and returns true if they are equal (case insensitive)


efficiently [](start = 24, length = 11)

oh the irony!

rajsesh · 2017-03-24T22:41:03Z

tests/Benchmark/KVOBenchmarkTests.mm

+
+    inline void Run() {
+        for (NSObject* obj in _testObservers.get()) {
+            [_testKVOObject addObserver:obj forKeyPath:@"intProperty" options:0 context:nullptr];


NSArray now has the helper function to add observers for every object in the arraay, so you can cut this loop.

Though that has the added cost of making an NSIndexSet* and is only a convenience wrapper

I think it would end up being harder to read because of the need to construct an NSIndexSet. Leaving as-is.

rajsesh · 2017-03-24T22:50:53Z

Frameworks/Foundation/NSKeyValueCoding.mm

-        if (quickSet<type>(self, setter, value, valueType)) { \
-            return true; \
-        } \
+    case encodingChar:                                            \


No changes other than formatting in this file?

Will back this out.

@aballway

- Fixed a bug in CFStringGetCStringPtr where it called _fastCStringContents with an NSString encoding, but _fastCStringContents expected a CFString encoding. As a result, CFStringGetCStringPtr now actually returns something for most strings. - Similarly, [NSString UTF8String] on a literal now returns a fast pointer rather than an autorelease buffer. - Changed [NSString isEqualToString:], [NSConstantString hash], and [NSConstantString isEqualToString:] to rely on faster functions - Made a similar change to speed up a local helper (sorry @aballway i misled you) - Changed [Class keyPathsForValuesAffectingKey:] to use C-string parsing - added a private helper for keyPathsForValuesAffectingKey: to not construct the empty set, for internal usage - Changed NSKVOKeypathObserver to store an observer as assign instead of as weak. This makes us theoretically less safe if an observer deallocs without removing itself as an observer. However, the observed object already threw once it reached its dealloc and still had observers, and the above case is considered incorrect anyway, so it only allowed us to fail quietly in the case where the observed object has a much longer lifetime, which is of questionable value. Opting in favor of assign here, as it provided a substantial (-~40%) decrease in time it takes to addObserver: - Changed _dispatchWillChange to just take an NSMutableDictionary instead of doing an unnecessary mutableCopy - Minor change to setObservationInfo: to be atomic on the associated object table rather than the object itself - Added more benchmark tests for KVO, String scenarios Fixes microsoft#2040

ms-jihua · 2017-03-27T17:08:22Z

Failure did not repro locally - i'll assume it's spurious and queue another CI build.

rajsesh · 2017-03-27T18:37:54Z

Frameworks/Foundation/NSString.mm

@@ -740,13 +743,11 @@ - (NSString*)description {
 @Status Interoperable
 */
 - (BOOL)isEqualToString:(NSString*)compStr {
-    RETURN_FALSE_IF(!compStr);
-
-    if ([compStr isKindOfClass:[NSString class]]) {


isKindOfClass [](start = 17, length = 13)

We can't use the short path for custom classes (such as NSStringOnHStting). Why is this removed?

am confused - NSHSTRINGString isKindOfClass:[NSString class] would have returned YES here...

Yes it would. Looking at CFStringCompareWithOptions, it can handle both non CF NSStrings as well as CFStrings, so this should be okay. Still confused about the original intent here :)

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

rajsesh · 2017-03-27T18:40:43Z

Frameworks/Foundation/NSKVOSupport.mm

+            // fast path using c string manipulation, will cover most cases, as most keyPaths are short
+            char selectorName[sc_bufferSize] = "keyPathsForValuesAffecting"; // 26 chars
+            selectorName[sc_prefixLength] = toupper(rawKey[0]);
+            strcat_s(&selectorName[sc_prefixLength + 1], sc_bufferSize - sc_prefixLength - 1, &rawKey[1]);


strcat_s [](start = 12, length = 8)

strcpy instead since you have already computed the offset.

rajsesh · 2017-03-27T18:45:55Z

Frameworks/Foundation/NSKVOSupport.mm

@@ -424,7 +450,7 @@ - (void*)observationInfo {
 @Status Interoperable
 */
 - (void)setObservationInfo:(void*)observationInfo {
-    objc_setAssociatedObject(self, &s_kvoObservationInfoAssociationKey, (__bridge id)observationInfo, OBJC_ASSOCIATION_RETAIN_NONATOMIC);
+    objc_setAssociatedObject(self, &s_kvoObservationInfoAssociationKey, (__bridge id)observationInfo, OBJC_ASSOCIATION_RETAIN);


OBJC_ASSOCIATION_RETAIN [](start = 102, length = 23)

why this subtle change here?

Changed to this from an @ synchronized elsewhere. More of a consistency change (this had maybe ~3% perf impact) - there was another call to this function that didn't do the synchronized for whatever reason, and the synchronization should probably occur within the body of this function rather than around it. If you want, I can change this to @ synchronized instead of using the associated objects table's locking, but I don't think it makes much of a difference.

rajsesh · 2017-03-27T19:13:08Z

Frameworks/Foundation/NSKVOSupport.mm

@@ -238,7 +264,7 @@ static void _addNestedObserversAndOptionallyDependents(_NSKVOKeyObserver* keyObs

    // Aggregate all keys whose values will affect us.
    if (dependents) {
-        NSSet* valueInfluencingKeys = [[object class] keyPathsForValuesAffectingValueForKey:key];
+        NSSet* valueInfluencingKeys = _keyPathsForValuesAffectingValueForKey([object class], key);


valueInfluencingKeys [](start = 15, length = 20)

could be nil. messages to nil object should be okay but i would double check this on ARM (we have had funky issues there but not with integral type rets).

seems to be fine - my only test failures were the CGGradient tests, which are a known issue. i doublechecked the nil message behavior in an isolated test as well.

rajsesh

Just a few minor comments.

bbowman · 2017-03-27T23:28:30Z

DHowett-MSFT · 2017-05-17T16:42:41Z

Frameworks/Foundation/NSKVOSupport.mm

@@ -573,7 +591,7 @@ - (void)willChangeValueForKey:(NSString*)key {
    if (![self observationInfo]) {
        return;
    }
-    _dispatchWillChange(self, key, @{ NSKeyValueChangeKindKey : @(NSKeyValueChangeSetting) });
+    _dispatchWillChange(self, key, [NSMutableDictionary dictionaryWithObject:@(NSKeyValueChangeSetting) forKey:NSKeyValueChangeKindKey]);


Oh no! @aballway this is what actually introduced the "information leak" I was talking about.

😝 I told ya it was already there

msftclas added the cla-not-required label Mar 24, 2017

ms-jihua added the Performance label Mar 24, 2017

aballway reviewed Mar 24, 2017

View reviewed changes

rajsesh reviewed Mar 24, 2017

View reviewed changes

ms-jihua force-pushed the kvo_perf4 branch from bab22c0 to 708f3ac Compare March 24, 2017 22:58

rajsesh reviewed Mar 27, 2017

View reviewed changes

rajsesh suggested changes Mar 27, 2017

View reviewed changes

strcpy

89bab76

rajsesh approved these changes Mar 27, 2017

View reviewed changes

ms-jihua merged commit 2f85b69 into microsoft:develop Mar 28, 2017

DHowett-MSFT mentioned this pull request Mar 31, 2017

Non-ASCII NSConstantStrings hash differently than their NSString counterparts. #2395

Closed

ms-jihua mentioned this pull request Mar 31, 2017

Make NSConstantString hashes match up with NSString hashes #2399

Merged

DHowett-MSFT reviewed May 17, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve perf in KVO and NSString #2323

Improve perf in KVO and NSString #2323

ms-jihua commented Mar 24, 2017

ms-jihua commented Mar 24, 2017 •

edited

Loading

aballway Mar 24, 2017

ms-jihua Mar 24, 2017

aballway Mar 24, 2017

ms-jihua Mar 24, 2017

aballway Mar 24, 2017

ms-jihua Mar 24, 2017

rajsesh Mar 24, 2017

ms-jihua Mar 24, 2017

rajsesh Mar 24, 2017

ms-jihua Mar 24, 2017

ms-jihua Mar 24, 2017 •

edited

Loading

rajsesh Mar 24, 2017

rajsesh Mar 24, 2017

rajsesh Mar 24, 2017

aballway Mar 24, 2017

ms-jihua Mar 24, 2017

rajsesh Mar 24, 2017

ms-jihua Mar 24, 2017

ms-jihua commented Mar 27, 2017

rajsesh Mar 27, 2017

ms-jihua Mar 27, 2017

rajsesh Mar 27, 2017

rajsesh Mar 27, 2017

rajsesh Mar 27, 2017

ms-jihua Mar 27, 2017

rajsesh Mar 27, 2017

ms-jihua Mar 27, 2017

rajsesh left a comment

bbowman commented Mar 27, 2017

DHowett-MSFT May 17, 2017

aballway May 17, 2017

		@@ -43,9 +43,7 @@
		* Helper function that efficiently compares two CFStringRef and returns true if they are equal (case insensitive)

Improve perf in KVO and NSString #2323

Improve perf in KVO and NSString #2323

Conversation

ms-jihua commented Mar 24, 2017

ms-jihua commented Mar 24, 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

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

ms-jihua Mar 24, 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

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

ms-jihua commented Mar 27, 2017

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

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

bbowman commented Mar 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ms-jihua commented Mar 24, 2017 •

edited

Loading

ms-jihua Mar 24, 2017 •

edited

Loading