-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Implements experimental diff with optimized moves calculation to produce only minimal required moves #1139
base: main
Are you sure you want to change the base?
Conversation
56a1ced
to
96f1a43
Compare
@tarbayev has updated the pull request. |
Generated by 🚫 Danger |
This is pretty incredible! Really excited to test this out. Before I dive too deep on the technical details, I'd love to first config this so that we can run an A/B test on it to measure performance gains (or losses) in Instagram.
I know its a little more work, but it'll give us evidence to how much memory and compute time this was taking. Not to mention giving us an escape hatch if something goes wrong. |
Amazing 💚 |
96f1a43
to
7042491
Compare
@tarbayev has updated the pull request. |
@rnystrom Here you go. It was quite a challenge to achieve this with a reasonable design, as I have very little experience with C++, so any suggestions for improvements are welcome. |
7042491
to
7a254bd
Compare
@tarbayev has updated the pull request. |
…uce only minimal required moves
7a254bd
to
6e173c2
Compare
Finally had a chance to run this in our project. Found a flaw in the algorithm which is now fixed, also the algorithm has become much simpler. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lorixx has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lorixx has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
@tarbayev Can you rebase again onto the latest master? I am having issue importing this PR, thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you rebase? Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lorixx has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
0179E635207FA8EB0082DFE6 /* IGListDiffExperimentOptimizedMovesTests.m in Sources */ = {isa = PBXBuildFile; fileRef = 0179E634207FA8EB0082DFE6 /* IGListDiffExperimentOptimizedMovesTests.m */; }; | ||
0179E636207FA8F30082DFE6 /* IGListDiffExperimentOptimizedMovesTests.m in Sources */ = {isa = PBXBuildFile; fileRef = 0179E634207FA8EB0082DFE6 /* IGListDiffExperimentOptimizedMovesTests.m */; }; | ||
0179E637207FA8F30082DFE6 /* IGListDiffExperimentOptimizedMovesTests.m in Sources */ = {isa = PBXBuildFile; fileRef = 0179E634207FA8EB0082DFE6 /* IGListDiffExperimentOptimizedMovesTests.m */; }; | ||
01A3FEE120FFB30100E91657 /* GameplayKit.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = 01A3FEE020FFB30000E91657 /* GameplayKit.framework */; }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like this framework may have been added by mistake?
@TimOliver has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
1 similar comment
@TimOliver has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@tarbayev Hi Nickolay! Sorry for the delay in getting back to you on this PR. I think this still has some value and I'd love to get it merged into IGListKit. Are you still open to that? I'm happy to take what you've added here and rebase it myself if you're okay with that, and don't want to spend anymore time on it. Thanks! |
@TimOliver Hi Tim, |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
I'm not really sure yet how to work this but I'm looking to check for profiles on insta and tiktok |
Changes in this pull request
Previously diff calculation resulted in redundant list of moves, for example diff for the following array pair:
[ 0, 1, 2, 3, 4, 5 ]
[ 5, 0, 1, 2, 3, 4 ]
produced moves:
5-0, 0-1, 1-2, 2-3, 3-4, 4-5
while the only required move was 5-0.
This leads to inefficient updates both by computation and memory consumption.
This may also cause crashes in UICollactionView or UITableView batch updates.
Checklist
CHANGELOG.md
for the enhancement.