Skip to content

Speed-up time-based samplers by 20X and index-based by 1.5X #284

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

Merged
merged 36 commits into from
Oct 24, 2024

Conversation

NicolasHug
Copy link
Member

@NicolasHug NicolasHug commented Oct 23, 2024

Fixes #256

We now let the samplers rely on our C++ "sort and dedup" logic, instead of the less efficient Python ones. This has a few benefits:

  • we can avoid extra copies
  • samplers can now return a 5D FrameBatch instead of a list of 4D FrameBatch. The 5D FrameBatch output is a "batch" of clips. Its data is of shape (num_clips, num_frames_per_clips, C, H, W) (or HWC).
  • the dedup logic now works efficicently for time based samplers (i.e. this fixes Time-based samplers are significantly slower than index-based samplers #256)

Running our samplers benchmark:

On main:
----------
num_clips = 1
clips_at_random_indices     med = 19.09ms +- 4.24
clips_at_regular_indices    med = 10.75ms +- 1.10
clips_at_random_timestamps  med = 17.47ms +- 4.45
clips_at_regular_timestamps med = 17.29ms +- 4.10
----------
num_clips = 50
clips_at_random_indices     med = 144.86ms +- 10.09
clips_at_regular_indices    med = 162.97ms +- 40.31
clips_at_random_timestamps  med = 2332.83ms +- 426.51
clips_at_regular_timestamps med = 1871.10ms +- 351.33



This PR:
----------
num_clips = 1
clips_at_random_indices     med = 15.27ms +- 4.07
clips_at_regular_indices    med = 8.70ms +- 1.33
clips_at_random_timestamps  med = 16.34ms +- 4.40
clips_at_regular_timestamps med = 9.57ms +- 3.69
----------
num_clips = 50
clips_at_random_indices     med = 97.06ms +- 3.90
clips_at_regular_indices    med = 107.23ms +- 2.73
clips_at_random_timestamps  med = 104.52ms +- 3.49
clips_at_regular_timestamps med = 117.30ms +- 5.80

🚀

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Meta Open Source bot. label Oct 23, 2024
Copy link
Member Author

Choose a reason for hiding this comment

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

Changes made to this file are only to:

  • assert the new output type
  • slightly udpate some parts to account for the new return type

I didn't add or remove any test

@NicolasHug NicolasHug marked this pull request as ready for review October 24, 2024 15:06
Copy link
Contributor

@ahmadsharif1 ahmadsharif1 left a comment

Choose a reason for hiding this comment

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

Tests are passing so this is great.

Non-blocking commentit would be nice to compare frame contents against reference clip contents gathered from the test data files (not comparing clip vs. another) for more confidence.

@NicolasHug
Copy link
Member Author

Thanks for the review! I'll follow-up with the tests you're mentioning soon

@NicolasHug NicolasHug merged commit ebc809c into pytorch:main Oct 24, 2024
40 checks passed
@NicolasHug NicolasHug deleted the samplers_hack branch October 24, 2024 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Meta Open Source bot.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Time-based samplers are significantly slower than index-based samplers
3 participants