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

Statically linked spin #2101

Merged
merged 6 commits into from
Dec 13, 2023

Conversation

rajatjindal
Copy link
Contributor

@rajatjindal rajatjindal commented Nov 16, 2023

For openshift integration we need statically linked binary of spin. This PR adds the required changes for that.

I think we also need to attach these artifacts to releases, I will look at it next (will add commits to this PR)

cc @0xE282B0

@rajatjindal rajatjindal force-pushed the statically-linked-spin branch from 7bf1319 to 76ff994 Compare December 8, 2023 06:12
@rajatjindal rajatjindal force-pushed the statically-linked-spin branch from da8a5bf to 18d1789 Compare December 8, 2023 07:17
@rajatjindal rajatjindal requested review from rylev and lann December 8, 2023 07:18
@rajatjindal rajatjindal merged commit c6ef3c2 into spinframework:main Dec 13, 2023
11 checks passed
@tschneidereit
Copy link
Contributor

While I wouldn't necessarily expect there to be problems, we should verify that these builds don't meaningfully regress performance. Musl isn't the fastest libc out there, and in particular its malloc implementation isn't fast, so that might result in unexpected regressions if we have hot code somewhere in Spin where that matters. (And if all of this isn't mitigated by Rust's Musl target anyway.)

@rajatjindal
Copy link
Contributor Author

Hi @tschneidereit, thank you for your feedback. Do you happen to have any suggestions about what could be the alternative here? I am happy to explore other options here.

@rajatjindal
Copy link
Contributor Author

also, just to ensure we are on same page, this PR adds additional release assets for statically linked Spin binaries (and does not replace existing releases binaries).

@tschneidereit
Copy link
Contributor

Oh sorry, I should've been clearer: I agree that having these builds makes sense! I also don't really have an alternative as such.

That doesn't mean that we shouldn't test performance of this configuration vs others though. If we were to discover meaningful regressions, there might be things we can do to address those without moving away from this target. For example, we could look into using a different allocator, or we could potentially change things in Spin to not be as affected by musl's lower performance.

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.

4 participants