Add ASExperimentalSkipClearData #trivial #1136
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nguyenhuy
approved these changes
Sep 19, 2018
Source/ASCollectionView.mm
Outdated
| @@ -576,6 +576,11 @@ - (void)setAsyncDelegate:(id<ASCollectionDelegate>)asyncDelegate | |||
| - (void)_asyncDelegateOrDataSourceDidChange | |||
| { | |||
| ASDisplayNodeAssertMainThread(); | |||
|
|
|||
| if (ASActivateExperimentalFeature(ASExperimentalSkipClearData)) { | |||
There was a problem hiding this comment.
Checking for nils is less expensive than checking for experiment, so let's do it first, e.g:
if (_asyncDataSource == nil && _asyncDelegate == nil && ASActivateExperimentalFeature(ASExperimentalSkipClearData)) {
[_dataController clearData];
}
maicki
force-pushed
the
MSAddASCollectionViewSkipClearDataExperiment
branch
from
5edc05b
to
38449ec
Compare
|
🚫 CI failed with log |
Generated by 🚫 Danger |
|
|
nguyenhuy
changed the title
nguyenhuy
pushed a commit
to nguyenhuy/Texture
that referenced
this pull request
Sep 20, 2018
* Add ASExperimentalSkipClearData * Move the experiment check within the if clause
nguyenhuy
added a commit
to nguyenhuy/Texture
that referenced
this pull request
Oct 2, 2018
This is a follow up on TextureGroup#1136. Our experiment results show that clearing data frequently is the cause of our TextureGroup#1 crash. @maicki and I believe that this is because if the collection view is being used, silently clearing its data without notifying the backing UICollectionView can put it out-of-sync and causes mayhem next time the collection view processes a batch update. If you look at the stack trace closely, you'll notice that the crash doesn't occur on the same run loop that clearData is called. This made it extremely tricky to investigate and identify the root cause. Another interesting question would be whether or not we want to clear the data during deallocation at all, since the data will be cleared out soon anyway.
mikezucc
pushed a commit
to mikezucc/Texture
that referenced
this pull request
Oct 2, 2018
* Add ASExperimentalSkipClearData * Move the experiment check within the if clause
nguyenhuy
added a commit
that referenced
this pull request
Oct 3, 2018
This is a follow up on #1136. Our experiment results show that clearing data frequently is the cause of our #1 crash. @maicki and I believe that this is because if the collection view is being used, silently clearing its data without notifying the backing UICollectionView can put it out-of-sync and causes mayhem next time the collection view processes a batch update. If you look at the stack trace closely, you'll notice that the crash doesn't occur on the same run loop that clearData is called. This made it extremely tricky to investigate and identify the root cause. Another interesting question would be whether or not we want to clear the data during deallocation at all, since the data will be cleared out soon anyway.
mikezucc
pushed a commit
to mikezucc/Texture
that referenced
this pull request
Nov 7, 2018
…1154) This is a follow up on TextureGroup#1136. Our experiment results show that clearing data frequently is the cause of our #1 crash. @maicki and I believe that this is because if the collection view is being used, silently clearing its data without notifying the backing UICollectionView can put it out-of-sync and causes mayhem next time the collection view processes a batch update. If you look at the stack trace closely, you'll notice that the crash doesn't occur on the same run loop that clearData is called. This made it extremely tricky to investigate and identify the root cause. Another interesting question would be whether or not we want to clear the data during deallocation at all, since the data will be cleared out soon anyway.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We are seeing crashes to an internal inconsistency deep within
UICollectionView. We were able to track down the range of commits where we start seeing this crash. To verify the hunch of the root cause of this crash we will introduce a new experiment that will skip calling clear data ifdataSource/delegateis cleared withinASCollectionView.