Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve Our Handling of Subnodes #223

Merged
merged 3 commits into from
May 2, 2017
Merged

Improve Our Handling of Subnodes #223

merged 3 commits into from
May 2, 2017

Conversation

Adlai-Holler
Copy link
Member

@Adlai-Holler Adlai-Holler commented May 2, 2017

  • Remove ASDisplayNode's conformance to NSFastEnumeration
    • We were creating a new copy of subnodes for each chunk of enumeration.
    • The enumerated collection wasn't stable – technically nodes could be deallocated during enumeration under the right circumstances.
  • Instead of making a new copy of subnodes each time, just store a cached copy.
  • Remove silly dealloc line that explicitly nils the ivar.

@Adlai-Holler
Copy link
Member Author

This is a breaking API change (removing the conformance) but I think the impact will be small because most people probably just iterate through node.subnodes.

@@ -127,6 +127,9 @@ FOUNDATION_EXPORT NSString * const ASRenderingEngineDidDisplayNodesScheduledBefo
ASDisplayNode * __weak _supernode;
NSMutableArray<ASDisplayNode *> *_subnodes;

// Set this to nil whenever you modify _subnodes
NSArray<ASDisplayNode *> *_cachedSubnodes;
Copy link
Member

@garrettmoon garrettmoon May 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a dangerous optimization. Could we at least put in an assert that _subnodes and _cachedSubnodes have the same content in the subnodes getter?

@Adlai-Holler Adlai-Holler merged commit b1e1bfd into master May 2, 2017
@Adlai-Holler Adlai-Holler deleted the AHBetterSubnoding branch May 2, 2017 20:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants