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.

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

perf: Fix cloud download speed regression #19734

Merged
merged 6 commits into from
Nov 12, 2024
Merged

Conversation

nameexhaustion
Copy link
Collaborator

@nameexhaustion nameexhaustion commented Nov 12, 2024

Reverts #19730 - from benchmarking it performed worse.

In hindsight it's not worth adding that rule - even without it we will only have up to half of our requests being tiny in the worst case (in this worst case they would be interleaved).

@nameexhaustion nameexhaustion added the skip changelog Do not include in changelog label Nov 12, 2024
@github-actions github-actions bot added performance Performance issues or improvements python Related to Python Polars rust Related to Rust Polars labels Nov 12, 2024
current_merged_range = range;
current_n_bytes = 0;
Some((v, current_idx))
current_n_bytes = current_merged_range.len();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fix incorrect re-initialization

@nameexhaustion nameexhaustion marked this pull request as ready for review November 12, 2024 03:48
@nameexhaustion nameexhaustion marked this pull request as draft November 12, 2024 04:16
@nameexhaustion nameexhaustion marked this pull request as ready for review November 12, 2024 04:59
@nameexhaustion nameexhaustion marked this pull request as draft November 12, 2024 06:25
let gap_tolerance =
(current_n_bytes.max(range.len()) / 8).clamp(1024 * 1024, 8 * 1024 * 1024);

leq_current_dist_to_chunk_size && distance <= gap_tolerance
Copy link
Collaborator Author

@nameexhaustion nameexhaustion Nov 12, 2024

Choose a reason for hiding this comment

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

The formatter broke with the existing if on the entire file 🥲, so I rewrote it to use descriptive variable names instead

@nameexhaustion nameexhaustion marked this pull request as ready for review November 12, 2024 06:31
@ritchie46 ritchie46 merged commit 36e5913 into pola-rs:main Nov 12, 2024
18 of 20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Performance issues or improvements python Related to Python Polars rust Related to Rust Polars skip changelog Do not include in changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants