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.

Sign up for GitHub

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

Jump to bottom

Implements experimental diff with optimized moves calculation to produce only minimal required moves #1139

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Implements experimental diff with optimized moves calculation to produce only minimal required moves #1139

wants to merge 1 commit into from

Conversation

tarbayev
Copy link

@tarbayev tarbayev commented Mar 27, 2018 •
edited

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

  • All tests pass. Demo project builds and runs.
  • I added tests and an experiment.
  • I added an entry to the CHANGELOG.md for the enhancement.
  • I have reviewed the contributing guide

@facebook-github-bot
Copy link
Contributor

@tarbayev has updated the pull request.

@iglistkit-bot
Copy link

iglistkit-bot commented Mar 27, 2018 •
edited

1 Warning
⚠️ All pull requests should have a milestone attached, unless marked #trivial.

Generated by 🚫 Danger

@rnystrom
Copy link
Contributor

rnystrom commented Mar 27, 2018 •
edited

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.

  • Add an entry to the experiments options
  • In the diffing algo, check the mask for this experimental feature
    • If exists, do these move calculations
    • Otherwise, fallback to previous versions

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.

@calimarkus
Copy link
Contributor

Amazing 💚

@facebook-github-bot
Copy link
Contributor

@tarbayev has updated the pull request.

@tarbayev tarbayev changed the title Fixes moves calculation to produce only minimal required moves Implements experimental diff with optimized moves calculation to produce only minimal required moves Apr 12, 2018
@tarbayev
Copy link
Author

@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.

@facebook-github-bot
Copy link
Contributor

@tarbayev has updated the pull request.

@tarbayev
Copy link
Author

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.

@rnystrom
Copy link
Contributor

@tarbayev awesome!! I’m sorry I let this slip too, on my queue

Sent with GitHawk

@wanhmr
Copy link

wanhmr commented Mar 19, 2019

@tarbayev @rnystrom Is there any update?

facebook-github-bot
facebook-github-bot reviewed Jun 24, 2019
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a 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.

facebook-github-bot
facebook-github-bot reviewed Jun 24, 2019
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a 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.

@lorixx
Copy link
Contributor

lorixx commented Jun 25, 2019

@tarbayev Can you rebase again onto the latest master? I am having issue importing this PR, thanks!

lorixx
lorixx requested changes Jun 25, 2019
View reviewed changes
Copy link
Contributor

@lorixx lorixx left a 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!

facebook-github-bot
facebook-github-bot reviewed Nov 26, 2019
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a 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.

iperry90
iperry90 reviewed Dec 17, 2019
View reviewed changes
IGListKit.xcodeproj/project.pbxproj
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 */; };
Copy link

@iperry90 iperry90 Dec 17, 2019 •
edited

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?

@facebook-github-bot
Copy link
Contributor

@TimOliver has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

1 similar comment
@facebook-github-bot
Copy link
Contributor

@TimOliver has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@tarbayev tarbayev closed this May 2, 2023
@TimOliver
Copy link
Member

TimOliver commented May 8, 2023 •
edited

@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 TimOliver reopened this May 8, 2023
@tarbayev
Copy link
Author

tarbayev commented May 8, 2023 •
edited

@TimOliver Hi Tim,
At some point I decided to make a Swift version of the algorithm, which I called OptiDiff and also improved and simplified it quite a lot. Hence I don't want to spend any more time on this PR, but you're free to merge it as well as make a C++ implementation of the OptiDiff if you like.

@heykomikmi

This comment was marked as off-topic.

Sign in to view
@heykomikmi

This comment was marked as off-topic.

Sign in to view
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iperry90 iperry90 iperry90 left review comments

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

@lorixx lorixx lorixx requested changes

Assignees
No one assigned
Labels
CLA Signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@tarbayev @facebook-github-bot @iglistkit-bot @rnystrom @calimarkus @wanhmr @lorixx @TimOliver @heykomikmi @iperry90