Skip to content

feat: Shrink trampoline size #3732

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

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

Conversation

magentaqin
Copy link

This PR is related to #2654

Optimization Result
Uncompressed size shrinks from 1.9 MB to 1.7 MB.
Before:
before-shrink
After:
after-shrink

Solution
1.use anyhow to replace miette
2.use pixi_exec_utils to replace pixi_utils
I found removing miette from the root level is not enough, as its dependency pixi_utils also relies on miette
To avoid impacting the whole crate pixi_utils, I tried to extract this file into another crate pixi_exec_utils and only import it without relying on miette
pixel_exec_utils

Things left and need to discuss further
1.I haven't extracted and fully replaced excutable_utils.rs with the newly created crate pixi_exec_utils, as I'm not sure whether this is the best solution and many other files depend on this file. So, it involves a lot of changes.
2.I haven't write tests for trampoline as this PR is still in progress
3. I think another issue that needs further discussion: which crate/binary file needs basic error info without miette, and which crate/binary file needs detailed error info and diagnosis with miette. If needed, I can create another issue to discuss this.

PR Checklist

  • I have thoroughly reviewed the code, focusing on its formatting, comments, tests.
  • I have confirmed that the code execution outputs are consistent with the old versions
  • The code is designed to be compatible on standard operating systems, including Windows, macOS, and Ubuntu.

@magentaqin magentaqin changed the title Shrink Trampoline Size feat: Shrink trampoline size May 5, 2025
@ruben-arts
Copy link
Contributor

Hello @magentaqin,

Thanks for the great PR! IMHO the change is not big enough to validate merged the changes right now and put time into the rest of the needed refactor. I would like to disucss a bigger overhaul of the trampoline to shrink the size even more. I feel we should get it below 1mb with ease.

Let's discuss that later ;)

@magentaqin
Copy link
Author

Hello @magentaqin,

Thanks for the great PR! IMHO the change is not big enough to validate merged the changes right now and put time into the rest of the needed refactor. I would like to disucss a bigger overhaul of the trampoline to shrink the size even more. I feel we should get it below 1mb with ease.

Let's discuss that later ;)

I agree that it needs a bigger refactor. Let's discuss that later!

@ruben-arts
Copy link
Contributor

Let's do that online when you joined Prefix.dev 🎉 I'll keep the PR open but move it to draft.

@ruben-arts ruben-arts marked this pull request as draft May 21, 2025 10:07
@magentaqin
Copy link
Author

Let's do that online when you joined Prefix.dev 🎉 I'll keep the PR open but move it to draft.

No Problem. 👌

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