Optimize layout flattening #982
Conversation
Adlai-Holler
requested review from
levi,
maicki,
garrettmoon,
appleguy and
nguyenhuy
|
|
||
| return YES; | ||
| } | ||
|
|
There was a problem hiding this comment.
Had to make this a method so that it can see the _sublayouts ivar for direct access (remove retain/release pair per sublayout).
| } | ||
| queue.insert(queue.cbegin(), sublayoutContexts.begin(), sublayoutContexts.end()); |
There was a problem hiding this comment.
The bottom here (the search expansion) is far and away the biggest win. We no longer create another vector (heap allocation) and we remove 2 retain/release pairs per sublayout. The first is inserting into the vector then destroying the vector, the second is inserting into the queue and then removing from the queue.
Source/Layout/ASLayout.mm
Outdated
| const NSArray<ASLayout *> *sublayouts = layout.sublayouts; | ||
| const NSUInteger sublayoutsCount = sublayouts.count; | ||
| unowned ASLayout *layout = context.layout; | ||
| const NSUInteger sublayoutsCount = layout->_sublayouts.count; |
There was a problem hiding this comment.
Direct ivar access isn't so much about avoiding the message-send, it's about avoiding the retain/release pair. _sublayouts is already a +1 so let's use it.
There was a problem hiding this comment.
Mind adding this comment in code?
| @@ -18,6 +18,7 @@ | |||
| #import <Foundation/NSObjCRuntime.h> | |||
|
|
|||
| #define AS_EXTERN FOUNDATION_EXTERN | |||
| #define unowned __unsafe_unretained | |||
There was a problem hiding this comment.
Not sure if we should just be consistent and use __unsafe_unretained in a framework.
There was a problem hiding this comment.
Pros:
- Less underscores
- Shorter, cleaner
Cons:
- Not standard
- Could one day be a collision and we'd have to revert
Source/Layout/ASLayout.mm
Outdated
| - (BOOL)isFlattened | ||
| { | ||
| // A layout is flattened if its position is null, and all of its sublayouts are of type displaynode with no sublayouts. | ||
| if (! ASPointIsNull(_position)) { |
Source/Layout/ASLayout.mm
Outdated
| return NO; | ||
| } | ||
|
|
||
| for (ASLayout *sublayout in _sublayouts) { |
There was a problem hiding this comment.
Would NSFastEnumeration allow us to cast that as unowned void * and cast down with a bridge to the type we need? In fact does it do a retain for the sub layouts in here?
There was a problem hiding this comment.
Fast enumeration doesn't retain the objects.
Source/Layout/ASLayout.mm
Outdated
| const NSArray<ASLayout *> *sublayouts = layout.sublayouts; | ||
| const NSUInteger sublayoutsCount = sublayouts.count; | ||
| unowned ASLayout *layout = context.layout; | ||
| const NSUInteger sublayoutsCount = layout->_sublayouts.count; |
There was a problem hiding this comment.
Mind adding this comment in code?
Add comment
* Optimize layout flattening * Changelog * Remove whitespace * Update ASLayout.mm Add comment
Layout flattening is one of the most common operations in our framework and it currently involves a ton of needless retain/release traffic (4 rr pairs per layout). This diff reduces that to 0.
Since I hope to make more use of the
__unsafe_unretainedqualifier in the future, I added anunownedalias for it, to make it prettier.Obviously please review this diff carefully, since no optimization is worth crashing.