Summary: This was a killswitch to be safe, so lets clean it up.
Differential Revision: D67072925
fbshipit-source-id: ac54972030e7d53e7abd95025e45773aa3c5efc6
Summary:
iOS 18 has new exceptions, which can be a bit tough to debug. The common ones we see:
1. Cell dequeue outside of -cellForIndex
2. Dequeue being called more than once
3. Returning a cell that was not dequeued
Lets add an assert for each. #1 and #2 can actually assert on the problematic call, which will give a nice stack strace. #3 will still have a generic stack trace, but at least we'll see the section-controller.
Differential Revision: D62809660
fbshipit-source-id: 454c44598d4b21e09e6f66eb63c502b1b966a993
Summary:
## Context
Merging multiple batch updates of the same section is inherently unsafe, because each block can mutate the underlaying list and lead to insert/deletes that operate on different before/after lists. The long term fix is to get rid of item level batch updates (insert/delete calls) and use diffing like for sections, but that's a much larger project.
Right now, we have a crash spiking because we're reloading the same cell multiple times. Each time `IGListDiff(...)` returns a pair of insert/delete for the same index (lets say 0). Once these updates are merge together, they look like this:
```
Item inserts = 0, 0
Item deletes = 0, 0
```
We dedupe deletes in `IGListBatchUpdateData`, but don't do anything for inserts, so it looks liket this:
```
Item inserts = 0, 0
Item deletes = 0
```
So we create an insert/delete unbalance and can crash. Tbh, not sure how this hasn't been a bigger issue in the past.
D21822502 tried to remove all duplicate inserts, but that caused other types of crashes, because multiple inserts are allowed.
## This diff
* Lets keep the net insert/delete count the same by keeping track of the number of deletes we remove and apply the same to inserts.
* Add some tests
* Add an experiment flag to kill this if needed to be safe. IG only: Note that I'm making the default true because the crash is happening pretty at start-up.
## Follow up
Over time, the batch update clean up logic was slowly added to multiple place (IGListBatchUpdateData, IGListAdapterUpdaterHelpers, IGListIndexPathResult), which is making it hard to follow. A first step would be to move it in 1 or 2 places and avoid redundant logic (like getting unique deletes). However, the real fix is getting rid of batch updates all together.
Differential Revision: D61719884
fbshipit-source-id: eda7c264c8239a6a106dbe0256fe777b38fae335
Summary:
By making `-performUpdate` completely synchronous sometimes, it unearthed a bug with the early exit where multiple updates can happen at once. Specifically, the shortcut doesn't block updates from being started within `-didUpdateToObject`.
Lets test removing it, since it's happening in a pretty sensitive part of updates.
Differential Revision: D60773894
fbshipit-source-id: 721dffcb6d5353d12693fa873eb0f502bcdf4c02
Summary: In case the collection-view is not visible, lets try batching more updates together to minimize the amount of off-screen work we do.
Differential Revision: D60691861
fbshipit-source-id: 154164fcd370e016c12bace60269c946a153c284
Summary:
Currently, we always call `dispatch_async` to coalesce updates, which slowing down the first update. Lets try a compromise where we don't delay the first update, but enforce a max update per time.
For example, lets say `minInterval = 10ms`, `intervalIncrement = 10 ms`, and `maxInterval = 30 ms` ...
* At 0ms, the first `-queueUpdate` is called, which calls `_performUpdate` synchronously
* At 2ms, the update is done
* At 5ms, a second `-queueUpdate` is called. Since it's been less than 10ms, we wait 5ms and increase the next window to 20ms.
* At 10ms, the second `_performUpdate` is called.
So effectively, we speed up the first `-queueUpdate`, but enforce a max update per ms. I'm not convinced we really need the increment, but I want to see what happens IRL.
Differential Revision: D60691863
fbshipit-source-id: 85faf2dbc38f754976843fff63e6eada4972581e
Summary: Dispatching to a background thread and back to main can slow things down, since we might have to wait on the main thread to be available. For small diffs, we might be better off just doing it on main. If this does well, we can expend on it, like taking into account device type.
Differential Revision: D60691859
fbshipit-source-id: e1776e1e3bf493d99fcc6fe8af6d4c01d60da660
Summary: `IGListKit` updates can block some interactions, so lets experiment with increasing the quality. Later, we'll decrease the quality if the `UICollectionView` is not visible.
Differential Revision: D60691858
fbshipit-source-id: 296715cac063eed4b2f4580950d5186f09893a39
Summary:
* Create `IGListAdaptiveDiffingExperimentConfig` which will hold all the experiment params
* Fork `IGListPerformDiffWithData` between the regular vs adaptive diffing path. Might seem overkill right now, but `_queueForData` will get more complicated in the following diffs.
Differential Revision: D60691864
fbshipit-source-id: 6f9adc9bceecee704980ff968cd734c77be57f26
Summary: QE launched and reduced crashes, so lets clean up
Differential Revision: D60184705
fbshipit-source-id: 574b0228e3efadf7bd3bd0677a4373a3b95888d1
Summary: QE is done, so lets clean it up
Reviewed By: DimaVartanian
Differential Revision: D60184706
fbshipit-source-id: 3ece0452386fe1a3daad1f8958a65700431db36c
Summary:
# Context
We've seen several experiments in which disabling standard-update animation yielded significant app-value wins. Overall there is a trend of moving towards snappier
- Reels grid disabled animation, led to vpvd wins
- Home feed disabled update animations -- led to Session and revenue wins
# This diff
From IGListAdapter+Common, create a kill switch to disable all animation. Potentially use this flag to dogfood and see where we have unnecessary animation.
Differential Revision: D54388375
fbshipit-source-id: a46c60944317db5deeceaac6d0baed71104d0c34
Summary:
## Issue
We're seeing crashes like this one:
```
Invalid batch updates detected: the number of sections and/or items returned by the data source before and after performing the batch updates are inconsistent with the updates.
Data source before updates = { 6 sections with item counts: [1, 0, 6, 8, 5, 7] }
Data source after updates = { 1 section with item counts: [0] }
Updates = [
]
```
I'm thinking the `IGListAdapter` gets deallocated during an update.
1. `1 section with item counts: [0]` is the default when the `self.collectionView.dataSource` is `nil`. I don't really see a way we'd return a single section with zero cells.
2. We're not seeing any of the additional crash information set by the `IGListAdapterUpdaterDelegate`, which would happen if `IGListAdapter` gets deallocated (it holds the strong pointer to the delegate).
## Fix
Lets keep a strong pointer within the scope of the function to make sure it sticks around. Theoretically, there could be a situation where the section-controller holds a strong pointer to the list-adapter in an update block, which gets released after it runs. This seems pretty unlikely, so lets test this fix to see if that's it.
Reviewed By: candance
Differential Revision: D52747014
fbshipit-source-id: 545aaa3deb90af85a011e716ac870659da42106f
Summary: This is probably not super safe, so lets kill it. If we want to know all the section and cell updates outside the update block, we need to make cells diffable, which is something we're considering.
Reviewed By: candance
Differential Revision: D52743451
fbshipit-source-id: fd8004876c452552931bf84ee73cd3ac15f3ba40
Summary: This was added in 2020 (D24173911), lets kill it if we're not testing or shipping.
Reviewed By: candance
Differential Revision: D52743453
fbshipit-source-id: 9d02bd32fc31db36333e436c6420836e48cbbceb
Summary: Stability metrics look neutral, so lets ship! This was expected since we had a fix on the product side already. The IGListKit fix prevents it from happening in the first place.
Reviewed By: candance
Differential Revision: D52743455
fbshipit-source-id: e92b2a578b993cb228bbe245c3a1a5ac3bdfe5cf
Summary:
Currently, we don't throw on `NSInternalInconsistencyException` as a temporary workaround, but there's a few issues with that:
1) `IGFailure` doesn't collect nearly as much information as exceptions, so it's difficult to debug. We could add more context to it, but feels like we should just fix the underlying issue instead, and remove the workaround.
2) If we throw in an animated update, all app animations break because `[UIView performWithoutAnimation:...]` doesn’t close properly.
3) Continuing to use the `UICollectionView` after it throws feels a bit risky IMO. There's no guarantee that it should work properly after that.
Lets reintroduce the exception slowly, fix each problem, and eventually remove the workaround.
In the future, we could do 2 other things:
1) Make `IGListExceptionDoctor` that can look at an exception's details and diagnose the issue, or at least provide more context.
2) Check if the updates make sense before telling the `UICollectionView`, so we can fallback to a `-reloadData`. This would shift the problem from a crash to a performance regression, so not ideal, but maybe less worse for production.
I'll add the experiment set-up in the next diffs.
Differential Revision: D50426255
fbshipit-source-id: 26e21d3dfcf4670ed07f397cf0d4decdda08eec5
Summary:
I'm looking at updating IGKitKit's test suite for Xcode 15, but when I went to do a build, it failed at the `IGWarnAssert` macro. Turns out that because we set the condition in the macro to always return `(NO)`, there's no need to expose `frmt` to test for the condition.
This diff removes `frmt` from the define, allowing the tests to build again.
Reviewed By: fabiomassimo
Differential Revision:
D49628618
Privacy Context Container: L1203725
fbshipit-source-id: b0456a149bf5be96c8401a21866880ccd72ebbcb
Summary: This assertion has been firing for at least the [last 3 months](https://fburl.com/scuba/errorreporting_instagram_ios_assertions/vfxo4h2g) and we have shipped the app anyways so it is definitely not launch blocking.
Reviewed By: fabiomassimo
Differential Revision: D49281317
fbshipit-source-id: d525f88fe676e813b8413a2e0f21b6b12728b359
Summary:
Follow up to D47263265
## What's happening
We're calling `listAdapter.reloadObjects(...)` in the middle of the IGListKit's internal data update, so it's using the new section index path, instead of the old one. So we're going from a list of 2, insert at 0, and try to reload index 1, then we crash (it'll try to update index 2, which doesn't exist in the old array)
## Fix
Just like for `[IGListAdapter performBatchAnimated:updates:completion:]`, lets use the old index. To get this working, lets ask the updater if we're in an update, rather than `IGListAdapter` try to keep track of it.
Sorry the diff is a bit long, but in case this feels reasonable to pick, I might
Differential Revision: D47281472
fbshipit-source-id: 193153cfc15c87084b67f58c50e921db459d6800
Summary:
## Changes in this pull request
Issue fixed: #
### Checklist
- [ ] All tests pass. Demo project builds and runs.
- [ ] I added tests, an experiment, or detailed why my change isn't tested.
- [ ] I added an entry to the `CHANGELOG.md` for any breaking changes, enhancements, or bug fixes.
- [ ] I have reviewed the [contributing guide](https://github.com/Instagram/IGListKit/blob/master/.github/CONTRIBUTING.md)
Pull Request resolved: https://github.com/Instagram/IGListKit/pull/1299
Reviewed By: m3rlin45
Differential Revision: D18703628
Pulled By: TimOliver
fbshipit-source-id: d347ed9c59c62ccdb73bd3abd14e289b7c06f3f6
Summary:
The standardized Meta copyright notice is "Copyright (c) Meta Platforms, Inc. and affiliates." and not "Copyright (c) Meta Platforms, Inc. and its affiliates." (Dropping the "its")
This diff updates the copyright notice in each source file to the correct this.
Reviewed By: willbailey
Differential Revision: D44737667
fbshipit-source-id: 643bf36df76723e70d9d826c53cf8f29b8a0c8cc
Summary:
A quick push to fix something I noticed while studying how IGListKit works. This simply replaces "Facebook, Inc" with "Meta Platforms, Inc" in all of the source files where the company copyright notice is posted. This should help bring our external facing projects more in line with our new corporate branding.
There's still a lot more references to "Facebook" as a company in the library (especially around linking to other Meta sponsored open source libraries), but this might need additional scrutiny and review on a case-by-case basis, so let's handle those ones separately.
Reviewed By: lorixx
Differential Revision: D41207363
fbshipit-source-id: 57cdbf5eb1023b41a5f32c0c05e01628686a19fe
Summary:
Chatted with maxolls about this assertion - it fires when a user of IGListKit accidentally implements a UICV layout (which IGListKit will ignore). Definitely not ideal, but not a launch blocking issue so demoting this assert to a warning.
It's currently the second highest firing mustfix in alpha 239.
https://fburl.com/scuba/errorreporting_instagram_ios_assertions/yap7opjy
Reviewed By: maxolls
Differential Revision: D36983661
fbshipit-source-id: 00497637cffea7553495c837bec267acca7cd3d8
Summary:
## Changes in this pull request
A better version of https://github.com/Instagram/IGListKit/issues/1465 =)
- SPM support with script-based generations.
- added macOS Catalyst support
### Generate SPM layout
1. From **project's root** run:
`bash scripts/generate_spm_sources_layout.sh`
2. Commit Changes
Repeat those steps each time you delete/add the project's files. **Make sure** to have this CI step which will check that `generate_spm_sources_layout.sh` is not broken.
Issue fixed: https://github.com/Instagram/IGListKit/issues/1368#1406
### Checklist
- [ ] All tests pass. Demo project builds and runs.
- [ ] I added tests, an experiment, or detailed why my change isn't tested.
- [ ] I added an entry to the `CHANGELOG.md` for any breaking changes, enhancements, or bug fixes.
- [ ] I have reviewed the [contributing guide](https://github.com/Instagram/IGListKit/blob/master/.github/CONTRIBUTING.md)
Pull Request resolved: https://github.com/Instagram/IGListKit/pull/1487
Reviewed By: DimaVartanian, candance
Differential Revision: D30428297
Pulled By: lorixx
fbshipit-source-id: 655291ff03445dec9b0b8cd97916f0c88207e9a7
Summary:
In T97824645 a test that was failing for other reason was fixed and started
failing because it could not compile IGListKit.
The problem is that IGListKit was not prepared to be compiled for Mac Catalyst
since it was only testing for iOS platforms to include UIKit. These changes
modify the header to work in Mac Catalyst.
Differential Revision: D30410680
fbshipit-source-id: f886826b9c485acfb039a9e681afd85f9186c344
Summary:
This was accidentally introduced in D25334717 (9cb16b0f31), at the time I didn't pay close attention to the impact there. The diff actually broke the CI for IGListKit in open source community since IGListDiffKit was meant to be compatible in both MacOS and iOS.
The right approach is to rely on the IGListCompatibility.h to resolve the linked code here.
Reviewed By: rzito
Differential Revision: D30344334
fbshipit-source-id: 842ca560a1bfe6d54e7d90466575e4ed35cd9b87
Summary:
`IGListExperimentBackgroundDiffing` has been running on Instagram for a couple months now, and we're seeing nice scroll performance improvements (less frame drops) without increasing crashes. Lets move the experiment to a real property!
Should it be enabled by default?
* I'm leaning toward no, but I could be convinced otherwise. This optimization is good for large/complex applications with lots of diffing, but I'm not sure the more common apps would need it initially. It does make updates more complicated and the order of operation will be a bit different. For example, we've seen some places call `-performUpdatesAnimated` and almost immediately expect the `IGListAdapter` data to be updated, leading to missing views. This issue can still happen without background diffing, but less often.
Reviewed By: joetam
Differential Revision: D25884786
fbshipit-source-id: 3c8755a774f63868b7dfbc8e7a2e5c012a9e7e27
Summary:
It's time to ship the new updater! `IGListExperimentalAdapterUpdater` has been running on Instagram for a couple months with better performance and stability. In this diff, we're renaming `IGListExperimentalAdapterUpdater` (and related classes) to `IGListAdapterUpdater`.
Here's a recap:
## Stability
* `[IGListAdapter setDataSource]` isn't safe
* We're changing the underlying data without telling the `UICollectionView`.
* Fix: Lets invalidate the `UICollectionView` data by changing its dataSource.
* `[IGListAdapter setCollectionView]` isn't safe
* This is synchronous, but we might have pending or on-going updates. The `UICollectionView` might get synced before the pending update actually start executing, so the diff results will be off.
* Fix: Lets wrap updates in a transaction that can be cancelled.
* Returning a nil `IGListSectionController` from `IGListAdapterDataSource` could crash
* The `IGListAdapterUpdater` will still perform the diffing assuming that all the objects will have a section, which isn't the case.
* Fix: Lets generate the `IGListSectionController` before the diffing.
* Other improvements
* Lets ask for the `fromObject` just before diffing, instead of asking when scheduling the update.
* If the `UICollectionView` section count doesn't match `fromObject`, lets fallback to a reload.
## Performance
* Re-test background diffing
* `IGListExperimentBackgroundDiffing` and coalescing updates wasn't safe because of the issues mentioned above. The longer we wait, the more likely we'll end up in a race condition. Lets try re-testing with the stability improvements.
* Unblocks background layout calculation
* This is a larger project, but these improvements are required to make background work safe.
* Only create the `backgroundView` if needed (although this doesn't really require the new updater)
## Other
* Transactions
* `IGListAdapterUpdater` is the workhorse of `IGListKit` and has become a bit hard to follow over the years. We want to break it apart into simpler, more manageable parts.
* Avoid blocks
* There's a lot of blocks flying around, making crash logs hard to read. Lets try to use methods/functions where possible.
Reviewed By: Haud, lorixx
Differential Revision: D25884782
fbshipit-source-id: 1357fa23513a239051d5b1766823effa3199f656
Summary: This makes sure the `UICollectionView` section count matches what we expect before applying a diff results, if not, we fallback to a `reloadData`. This doesn't decrease the crash rate significantly, but it's nice to have a last line of defense. We do tradeoff crashes for performance issues, but it's better that an app works at all and we'll see the issue via asserts.
Reviewed By: lorixx
Differential Revision: D25884778
fbshipit-source-id: 5011d0907ce0f971ea3a0bf95c1549d52f615982
Summary: This experiment shows a slight decrease in frame-drops on features that have more expensive hashing, so lets ship!
Reviewed By: lorixx
Differential Revision: D25884785
fbshipit-source-id: a5e33abe6f129166ab9b75de80636db801599b74
Summary: `NSIndexPath` is in UIKit, so we should be importing its header.
Reviewed By: lorixx
Differential Revision: D25334717
fbshipit-source-id: 5a134a0ddbe88fddb33adfa182d7aaf437bc47d0
Summary:
Another source-of-truth inconsistency issue.
I kept getting this assertion T65423827 task.
It turned out the the root cause is that we use the `_viewSectionControllerMap` to keep track of the mapping between view and section controller.
Which is dangerous.
Because the UICV's source of truth is the `sectionMap`, now we are adding another source of truth between view <-> sectionController to be `_viewSectionControllerMap`, this is just waiting for bugs to happen.
We should always refers to sectionMap as the source of truth for everything here: since we can get the section index, we can replace all the `-sectionControllerForView` to use `-sectionControllerForSection:` instead, which is safer.
Multiple source of truth is the bug for this unexpected assertion.
Once we can cleanup this, then we dont need to use that weird: `- (void)mapView:(UICollectionReusableView *)view toSectionController:(IGListSectionController *)sectionController` API which we might forget to map, that can cause bug.
Reviewed By: elfredpagan
Differential Revision: D24173911
fbshipit-source-id: 6138cafc0b382fc569d17b14b13a6b50d85d342d
Summary: We don't actually need to call `[UICollectionVew performBatchUpdates ...]` if we don't have changes.
Reviewed By: patters
Differential Revision: D23386181
fbshipit-source-id: 469a22a35b9ead5181e95f36a8bc59ba36373bbe
Summary:
We're seeing some crashes where the `UICollectionView`'s section count doesn't match the `fromObjets`. Ideally that shouldn't happen, but in case it does, lets fallback to a `reloadData` instead of crashing. Lets `IGFailAssert()` to keep an eye on it.
EDIT: Turns out `UICollectionView` will sometimes already do that for item-level inconsistencies! Example error message:
> The number of items contained in an existing section after the update (9) must be equal to the number of items contained in that section before the update (10), plus or minus the number of items inserted or deleted from that section (0 inserted, 0 deleted) and plus or minus the number of items moved into or out of that section (0 moved in, 0 moved out). - will perform reloadData.
But it still throws a `NSInternalInconsistencyException` for section-level inconsistencies.
Reviewed By: patters
Differential Revision: D23386178
fbshipit-source-id: a9930b619701cffd37d0d6e2bcb1268ef7c9d0ce
Summary:
For diffing, `IGListAdapterUpdater` uses the full `fromObjects` instead of `validObjects`, which can be different if we're missing `IGListSectionController`. The diffing results won't match what the `IGListAdapter` tells the `UICollectionView`, so we can crash.
To fix this, lets try to generate the `IGListSectionController` before performing the diffing, and keep them in a container object (`IGListTransitionData`) alonside the `fromObjects` and `toObjects`. This might also unblock other cool features in the future, like giving `IGListSectionControllers` time for background work, but that's for another day.
Since this change is relatively large, lets create a temporary protocol `IGListUpdatingDelegateExperimental` which will have a slightly different API from `IGListUpdatingDelegate`. When/if the new implementation ships, we can merge both protocols.
Why not edit `IGListAdapterUpdater` directly?
* I don't want to mess up exiting updaters just yet.
Reviewed By: patters
Differential Revision: D23145776
fbshipit-source-id: 21642cafa64e2a26a173e2ba683ef5c6cede17d7
Summary: We pass `experiments` to places that don't need it, so lets clean that up.
Reviewed By: patters
Differential Revision: D23145782
fbshipit-source-id: affcfe0f38d1b8e544940af89e5692dbdc36dd76
Summary:
On `IGListAdapater` data update:
1) Pass the capacity count, so that arrays don't have to re-size.
2) Avoid using a set, so that we don't need to deal with hashes and equality. The updater should have dealt with duplicates already.
Reviewed By: patters
Differential Revision: D23145771
fbshipit-source-id: 2ed93231e15ddcd66cfe4d1f7384c563c77caa8e
Summary: We call `[collectionView layoutIfNeeded]` before performing updates, but it's not clear why. It leads to some strange animation issues when performing updates while scrolling. So lets try to skip it?
Reviewed By: Haud
Differential Revision: D22244053
fbshipit-source-id: 0c01a73860ebc16f430fed04d5c29a5bde038286
Summary: This experiment shipped so we can remove it. We still need a way to disable it with `allowsReloadingOnTooManyUpdates` in case we don't want to resign the first responder on large updates.
Reviewed By: Haud
Differential Revision: D22219238
fbshipit-source-id: 98e78f3ed040809db6c4b4c8da8fd0e976aca0a2
Summary: Actually this won't work and causes even more crashes. For example, if we reload cell 0 and insert at 0, we end up with just a single delete & insert at 0, which causes an inconsistency exception. `IGListApplyUpdatesToCollectionView` already takes care of avoiding multiple reloads of the same `NSIndexPath` by using a `NSMutableSet` when collecting the `reloadDeletePaths`.
Reviewed By: apadalko
Differential Revision: D21822502
fbshipit-source-id: c2382951ffe013f82c1de2492286d2911f40444d
Summary: Ship this experiment. `IGListCollectionViewLayout` should get the section/index counts via `UICollectionView` to stay in sync, instead of the `dataSource`.
Reviewed By: apadalko
Differential Revision: D21789871
fbshipit-source-id: 9fe069bd793b36680cd8562e69c7367d69a11ec7
