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

Hc patchfrom part2 #4279

Closed
wants to merge 12 commits into from
Closed

Hc patchfrom part2 #4279

wants to merge 12 commits into from

Conversation

Cyan4973
Copy link
Contributor

@Cyan4973 Cyan4973 commented Feb 1, 2025

just checking CI

improves compression ratio at low levels
by avoiding to duplicate in memory
a dictionary that was passed by reference.
thus saving a bit of memory and a little bit of cpu time
--patch-from no longer blocked on first job dictionary loading
notably when first job takes too long to load its prefix
@Cyan4973
Copy link
Contributor Author

Cyan4973 commented Feb 1, 2025

The new MT strategy in this PR favors keeping the jobs busy,
while the previous approach favored keeping I/O maximally completed.

As a consequence, this has an impact on the state of internal buffers at wake-up events,
and the --adapt mode, which was depending on them, doesn't work anymore.

This leaves a few option:

  • Do not change the MT mode
  • Do no longer support --adapt.
  • Refactor the MT mode to make it compatible with --adapt
  • Refactor --adapt to no longer depend on internal buffer state.

The last 2 items obviously cost a lot more time than the first 2 ones,
and this unfortunately may be the most critical ingredient in this equation.

which is transferred to the current oldest unfinished job.
resulting in less "wasted" idle time
@Cyan4973
Copy link
Contributor Author

Cyan4973 commented Feb 2, 2025

Changed for a combined strategy of waking up on flush event + waking up on job slot availability.
This requires sharing a condition variable, which is more complex to setup.

@Cyan4973
Copy link
Contributor Author

Cyan4973 commented Feb 3, 2025

This PR won't be merged. It's too complex and not stable enough.

I'll see if some of the simpler ideas can provide benefits and create a separate PR with them if that's the case.

@Cyan4973 Cyan4973 closed this Feb 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants