Reimplement ASRectTable using unordered_map to avoid obscure NSMapTable exception. #719

appleguy · 2017-12-18T03:27:20Z

The new class is called ASRectMap, which patterns alongside ASIntegerMap in both name and implementation.

Here's the exception, which occurs about 50 times a day in my app, when invoked from the only current caller (ASLayout):

"*** -[NSMapTable initWithKeyPointerFunctions:valuePointerFunctions:capacity:] Requested configuration not supported."

After some pretty detailed investigation, including study of open-source reimplementations
of Foundation, the best lead I've found on the NSMapTable exception is that
some NSPointerFunction types are not fully supported. Strangely, the ones being used
do seem to work fine almost all of the time.

The main concern is the Struct memory type, which is not officially re-declared in
NSMapTable, and as such the documentation claims that there may exist some
combinations of NSPointerFunction that are not supported. One substantiation for this is that crashes are disproportionately more common since 11.1.2, suggesting there might be a framework change to the concrete subclass in how it handles these flags. Unfortunately, I have not been able to reproduce, but I have ruled out many possibilities such as devices low on memory.

Because the exception is occurring frequently enough to be a concern (in the hundreds
to low thousands, though only 50 a day) - I decided to replace NSMapTable entirely
in order to ensure full correctness.

This does lose some features of the original - namely, strong retaining of keys. However, this feature was unused and the class is private. Since the previous implementation is operating in a grey area of NSMapTable's API, switching to unordered_map seemed like the right tradeoff.

ghost · 2017-12-18T03:28:50Z

🚫 CI failed with log

…le exception. The new class is called ASRectMap, which patterns alongside ASIntegerMap in both name and implementation. After some pretty detailed investigation, including study of open-source reimplementations of Foundation, the best lead I've found on the NSMapTable exception is that some NSPointerFunction types are not fully supported. Strangely, the ones being used do seem to work fine almost all of the time. The main concern is the Struct memory type, which is not officially re-declared in NSMapTable, and as such the documentation claims that there may exist some combinations of NSPointerFunction that are not supported. Because the exception is occurring frequently enough to be a concern (in the hundreds to low thousands, though only 50 a day) - I decided to replace NSMapTable entirely in order to ensure full correctness. "*** -[NSMapTable initWithKeyPointerFunctions:valuePointerFunctions:capacity:] Requested configuration not supported."

ghost · 2017-12-18T03:34:38Z

🚫 CI failed with log

appleguy · 2017-12-18T03:39:13Z

This builds fine on my local machine, and the tests execute. I think the build server has some kind of caching problem where the file rename is not being picked up correctly. Any suggestions?

maicki

Cool. Straight forward implementation. Looks good to me!

maicki · 2017-12-18T17:38:37Z

Source/Private/ASRectMap.mm

+#import <unordered_map>
+
+@implementation ASRectMap {
+  std::unordered_map<void *, CGRect> _map;


We may should add a comment that we use a void * key here as arc would not be able to determine the ownership qualification.

Could we not use __unsafe_unretained id as the key type here?

Unfortunately I don't think we can use __unsafe_unretained id as key. The unordered_map template cannot figure out the hash for the __unsafe_unretained id key. We would have to write our own key like:

struct ASRectMapMapKey { __unsafe_unretained id obj; bool operator==(const ASRectMapMapKey &other) const { return [obj isEqual:other.obj]; } }; namespace std { template<> struct hash<ASRectMapMapKey> { size_t operator()(const ASRectMapMapKey &k) const { return [k.obj hash]; } }; }

And we could use it via:

std::unordered_map<ASRectMapMapKey, CGRect> map; map.insert({{@"key"}, CGRectMake(0, 0, 100, 100)}); NSLog(@"%@", NSStringFromRect(map.at({@"key"})));

maicki · 2017-12-18T17:41:47Z

Source/Private/ASRectMap.mm

+
+  // { ptr1->rect1 ptr2->rect2 ptr3->rect3 }
+  NSMutableString *str = [NSMutableString string];
+  for (auto it = _map.begin(); it != _map.end(); it++) {


You could also use of a bit of a smaller code:

for ( auto const& entry : _map ) { [str appendFormat:@" %@->%@", entry->first, NSStringFromCGRect(entry->second)]; }

Is this method dangerous, now that the keys are stored unretained? Could we be calling -description on a garbage pointer? We could fall back to %p.

Agree with @Adlai-Holler. Let's pay extra attention here.

Adlai-Holler

Nice! A few notes and then let's go.

Adlai-Holler · 2017-12-18T17:44:44Z

Source/Private/ASRectMap.mm

+#import <unordered_map>
+
+@implementation ASRectMap {
+  std::unordered_map<void *, CGRect> _map;


Could we not use __unsafe_unretained id as the key type here?

Adlai-Holler · 2017-12-18T17:46:37Z

Source/Private/ASRectMap.mm

+
+  // { ptr1->rect1 ptr2->rect2 ptr3->rect3 }
+  NSMutableString *str = [NSMutableString string];
+  for (auto it = _map.begin(); it != _map.end(); it++) {


Is this method dangerous, now that the keys are stored unretained? Could we be calling -description on a garbage pointer? We could fall back to %p.

Adlai-Holler · 2017-12-18T17:48:13Z

Source/Private/ASRectMap.mm

+  }
+  [result addObject:@{ @"ASRectMap": str }];
+
+  return result;


The class name will already be in the description by default, so I think what we want here is @{ (id)kCFNull : str } which will just dump the list of pointers inside e.g.: <ASRectMap: 0xFFFFFFFF; "a->b, c->d">

Edit: NM I see you used MakeWithoutObject so we exclude the pointer from the description a la other collection classes 👍

nguyenhuy · 2017-12-19T17:07:33Z

Source/Private/ASRectMap.mm

+
+  // { ptr1->rect1 ptr2->rect2 ptr3->rect3 }
+  NSMutableString *str = [NSMutableString string];
+  for (auto it = _map.begin(); it != _map.end(); it++) {


Agree with @Adlai-Holler. Let's pay extra attention here.

nguyenhuy · 2017-12-19T17:11:05Z

Source/Private/ASRectMap.mm

+//  You may obtain a copy of the License at
+//
+//      http://www.apache.org/licenses/LICENSE-2.0
+//


Please update this file license per Danger's comment.

// // ASRectMap.mm // Texture // // Copyright (c) 2017-present, Pinterest, Inc. All rights reserved. // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. // You may obtain a copy of the License at // // http://www.apache.org/licenses/LICENSE-2.0 //

ghost · 2017-12-19T17:53:22Z

🚫 CI failed with log

appleguy · 2017-12-19T20:41:58Z

Thanks for all the feedback, will do some updates! Any ideas on why the build system is failing?

ghost · 2017-12-19T20:49:12Z

🚫 CI failed with log

appleguy · 2017-12-19T20:49:17Z

OK, ready for another review - if there are any ways to improve the safety of the accesses, let me know (I didn't quite understand if there is).

I considered using __unsafe_unretained id as the argument type for the methods, but this wouldn't affect any behavior, and would just be a form of API documentation — I can make that change if it seems better.

Main thing for now is getting the build to pass, which since I don't have admin access to the system, I need help to do.

nguyenhuy · 2017-12-20T19:26:13Z

OK, tests are all green now. Let's have an approval from @Adlai-Holler before merging.

Adlai-Holler

Everything is in order! Nice improvement ⚡️

appleguy · 2017-12-22T00:17:21Z

Thanks everyone!

…le exception. (TextureGroup#719) * Reimplement ASRectTable using unordered_map to avoid obscure NSMapTable exception. The new class is called ASRectMap, which patterns alongside ASIntegerMap in both name and implementation. After some pretty detailed investigation, including study of open-source reimplementations of Foundation, the best lead I've found on the NSMapTable exception is that some NSPointerFunction types are not fully supported. Strangely, the ones being used do seem to work fine almost all of the time. The main concern is the Struct memory type, which is not officially re-declared in NSMapTable, and as such the documentation claims that there may exist some combinations of NSPointerFunction that are not supported. Because the exception is occurring frequently enough to be a concern (in the hundreds to low thousands, though only 50 a day) - I decided to replace NSMapTable entirely in order to ensure full correctness. "*** -[NSMapTable initWithKeyPointerFunctions:valuePointerFunctions:capacity:] Requested configuration not supported." * Fix Xcode project

appleguy self-assigned this Dec 18, 2017

appleguy requested review from nguyenhuy and Adlai-Holler December 18, 2017 03:27

appleguy force-pushed the ASRectTable branch from 883abb7 to 351d9e4 Compare December 18, 2017 03:33

maicki approved these changes Dec 18, 2017

View reviewed changes

Adlai-Holler reviewed Dec 18, 2017

View reviewed changes

nguyenhuy approved these changes Dec 19, 2017

View reviewed changes

Merge branch 'master' into ASRectTable

e254ddc

Fix Xcode project

d171da4

Adlai-Holler approved these changes Dec 22, 2017

View reviewed changes

appleguy merged commit 131619d into master Dec 22, 2017

appleguy deleted the ASRectTable branch December 22, 2017 00:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reimplement ASRectTable using unordered_map to avoid obscure NSMapTable exception. #719

Reimplement ASRectTable using unordered_map to avoid obscure NSMapTable exception. #719

appleguy commented Dec 18, 2017 •

edited

Loading

ghost commented Dec 18, 2017

ghost commented Dec 18, 2017

appleguy commented Dec 18, 2017

maicki left a comment

maicki Dec 18, 2017

Adlai-Holler Dec 18, 2017

maicki Dec 18, 2017 •

edited

Loading

maicki Dec 18, 2017 •

edited

Loading

Adlai-Holler Dec 18, 2017

nguyenhuy Dec 19, 2017

Adlai-Holler left a comment

Adlai-Holler Dec 18, 2017

Adlai-Holler Dec 18, 2017

Adlai-Holler Dec 18, 2017 •

edited

Loading

nguyenhuy Dec 19, 2017

nguyenhuy Dec 19, 2017

ghost commented Dec 19, 2017

appleguy commented Dec 19, 2017

ghost commented Dec 19, 2017

appleguy commented Dec 19, 2017

nguyenhuy commented Dec 20, 2017

Adlai-Holler left a comment

appleguy commented Dec 22, 2017

Reimplement ASRectTable using unordered_map to avoid obscure NSMapTable exception. #719

Reimplement ASRectTable using unordered_map to avoid obscure NSMapTable exception. #719

Conversation

appleguy commented Dec 18, 2017 • edited Loading

ghost commented Dec 18, 2017

ghost commented Dec 18, 2017

appleguy commented Dec 18, 2017

maicki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maicki Dec 18, 2017 • edited Loading

Choose a reason for hiding this comment

maicki Dec 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Adlai-Holler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Adlai-Holler Dec 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Dec 19, 2017

appleguy commented Dec 19, 2017

ghost commented Dec 19, 2017

appleguy commented Dec 19, 2017

nguyenhuy commented Dec 20, 2017

Adlai-Holler left a comment

Choose a reason for hiding this comment

appleguy commented Dec 22, 2017

appleguy commented Dec 18, 2017 •

edited

Loading

maicki Dec 18, 2017 •

edited

Loading

maicki Dec 18, 2017 •

edited

Loading

Adlai-Holler Dec 18, 2017 •

edited

Loading