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

Reduce latency when loading data from external loaders #4797

Merged
merged 2 commits into from Jan 15, 2024
Merged

Reduce latency when loading data from external loaders #4797

merged 2 commits into from Jan 15, 2024

Conversation

teh-cmc
Copy link
Member

@teh-cmc teh-cmc commented Jan 12, 2024 •
edited by github-actions bot

External loaders are streaming their data via stdout as expected, but we're waiting for their exit status code in order to decide whether they are compatible... which nullifies the whole point of streaming the data in the first place!

This PR now detects as soon as a child process starts streaming data in, and considers that as a sure sign of compatibility.

0.12:

24-01-12_12.09.33.patched.mp4

Now:

24-01-12_12.10.00.patched.mp4

Checklist

  • I have read and agree to Contributor Guide and the Code of Conduct
  • I've included a screenshot or gif (if applicable)
  • I have tested the web demo (if applicable):
  • The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG

@teh-cmc teh-cmc added 🪳 bug Something isn't working 😤 annoying Something in the UI / SDK is annoying to use 📉 performance Optimization, memory use, etc include in changelog labels Jan 12, 2024
@nikolausWest nikolausWest added this to the 0.12.1 milestone Jan 12, 2024
@nikolausWest
Copy link
Member

Nice! This is pretty motivating for doing a patch release

emilk
emilk approved these changes Jan 12, 2024
View reviewed changes
Copy link
Member

@emilk emilk left a comment

Choose a reason for hiding this comment

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

Excellent!

crates/re_data_source/src/data_loader/loader_external.rs Outdated Show resolved Hide resolved
crates/re_data_source/src/data_loader/loader_external.rs Outdated Show resolved Hide resolved
crates/re_data_source/src/data_loader/loader_external.rs Show resolved Hide resolved
@teh-cmc teh-cmc merged commit da8976d into main Jan 15, 2024
40 checks passed
@teh-cmc teh-cmc deleted the cmc/immediate_feedback branch January 15, 2024 16:06
emilk pushed a commit that referenced this pull request Jan 17, 2024
@teh-cmc @emilk
Immediate compatiblity feedback for external loaders (#4797)
6bf4d44 
External loaders are streaming their data via stdout as expected, but
we're waiting for their exit status code in order to decide whether they
are compatible... which nullifies the whole point of streaming the data
in the first place!

This PR now detects as soon as a child process starts streaming data in,
and considers that as a sure sign of compatibility.

`0.12`:



https://github.com/rerun-io/rerun/assets/2910679/872d7bc3-f907-4dd4-9cbe-c4c85510ffc1



Now:
 


https://github.com/rerun-io/rerun/assets/2910679/4266c194-1eb0-4c34-a79a-c2f4286688ba
@emilk emilk changed the title Immediate compatiblity feedback for external loaders Reduce latency when loading data from external loaders Jan 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emilk emilk emilk approved these changes

Assignees
No one assigned
Labels
😤 annoying Something in the UI / SDK is annoying to use 🪳 bug Something isn't working include in changelog 📉 performance Optimization, memory use, etc
Projects
None yet
Milestone
0.12.1
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@teh-cmc @nikolausWest @emilk