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

[DNM] cramjam support #7823

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

[DNM] cramjam support #7823

wants to merge 4 commits into from

Conversation

crusaderky
Copy link
Collaborator

Blocked by

  • cramjam 2.7.0 final release
  • A/B tests

@crusaderky crusaderky self-assigned this May 10, 2023
Comment on lines +91 to +92
# TODO change to 2.7.0. This is a hack to make it work with 2.7.0-rc3.
if parse_version(cramjam.__version__) < parse_version("2.6.99"):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

DNM

@github-actions
Copy link
Contributor

github-actions bot commented May 10, 2023

Unit Test Results

See test report for an extended history of previous test failures. This is useful for diagnosing flaky tests.

       25 files   -        1         25 suites   - 1   14h 42m 55s ⏱️ - 1h 29m 52s
  3 608 tests +     27    3 499 ✔️ +     29     105 💤 ±0  4  - 2 
42 399 runs   - 2 941  40 223 ✔️  - 2 946  2 172 💤 +9  4  - 4 

For more details on these failures, see this check.

Results for commit 9b488a7. ± Comparison against base commit 09ea0f5.

♻️ This comment has been updated with latest results.

@crusaderky
Copy link
Collaborator Author

The whole decompress_into hack would become redundant if we could get rid of sharding.
XREFS:

@crusaderky
Copy link
Collaborator Author

crusaderky commented May 10, 2023

The A/B tests show a >2x slowdown of cramjam.lz4 vs. plain lz4. Either that, or one of the test modules hangs forever. Did not investigate yet.
https://github.com/coiled/benchmarks/actions/runs/4938359937/jobs/8828024130

@milesgranger
Copy link
Contributor

The A/B tests show a >2x slowdown of cramjam.lz4 vs. plain lz4.

oi, got me nervous. Ran benchmarks again on cramjam's end and all seems normal, as well as your example comparison and got:

=== lz4 ===
0.5676321685314178
75.5 ms ± 574 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)
44.8 ms ± 509 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)
=== cramjam.lz4 ===
0.5676321685314178
64.3 ms ± 156 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)
24.8 ms ± 309 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)
9.74 ms ± 22.1 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)

Nothing immediately stands out to me here why it'd be slower though, but weird edge cases happen. 🤔

@crusaderky
Copy link
Collaborator Author

crusaderky commented May 15, 2023 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants