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

Reducing ArmV8 binary build time with action-rs (cross build with Rust) #1811

Merged
merged 7 commits into from Oct 24, 2021

Conversation

patrickdung
Copy link
Contributor

This pull request is based on discussion #1790

Note:

  1. The binaries of this PR is additional to existing binary built
    Existing binary would be produced (by existing GitHub workflow/action)

meilisearch-linux-amd64
meilisearch-linux-armv8
meilisearch-macos-amd64
meilisearch-windows-amd64.exe
meilisearch.deb

  1. This PR produce these binaries. The name 'meilisearch-linux-aarch64' is used to avoid naming conflict with 'meilisearch-linux-armv8'.

meilisearch-linux-aarch64
meilisearch-linux-aarch64-musl
meilisearch-linux-aarch64-stripped
meilisearch-linux-amd64-musl

  1. If it's fine (in next release), we should submit another PR to stop generating meilisearch-linux-armv8 (which could take two to three hours to build it)

@patrickdung patrickdung changed the title Reducing ArmV8 binary build time with action-rs (cross build with Rust) #1790 Reducing ArmV8 binary build time with action-rs (cross build with Rust) Oct 14, 2021
@patrickdung patrickdung marked this pull request as ready for review October 14, 2021 15:34
@curquiza curquiza linked an issue Oct 14, 2021 that may be closed by this pull request
@curquiza
Copy link
Member

Thanks @patrickdung we will review this asap :)

@curquiza curquiza added this to the v0.24.0 milestone Oct 14, 2021
@curquiza
Copy link
Member

Hello @patrickdung
sorry for the delay and thanks again for the PR

I discussed with the team, and we don't want to provide

  • the musl binary: this can bring to performance loss. Also it could lead us to do more maintenance only for those binaru and this is not what we want at the moment. But thanks for the suggestion.
  • the stripped binary: we don't want to provide binaries where we cannot track the bug since we need bug reports to improve our product.

However we agree to keep the meilisearch-linux-aarch64 binary and replace the ARMv8 one in the future :)

@patrickdung
Copy link
Contributor Author

Hello @curquiza,

The Musl binaries is a proof of concept about the action-rs rust cross build platform. It could build/cross-build Musl binaries on a X86_64 system. It's fine taking it out.

BTW, I am not sure about the binary used in Meilisearch docker image is a Musl binary, Glibc binary or support both C libraries (hybrid build?):

$ ldd meilisearch
        linux-vdso.so.1 (0x00007ffce78ee000)
        libgcc_s.so.1 => ./libgcc_s.so.1 (0x00007f70ec05e000)
        libc.musl-x86_64.so.1 => not found
        libc.so.6 => /lib64/libc.so.6 (0x00007f70ebe24000)
        /lib/ld-musl-x86_64.so.1 => /lib64/ld-linux-x86-64.so.2 (0x00007f70ee520000)

Finally, check out this link for good performance with Microsoft mimalloc with Musl.

OK, looks like the stripped binary (did not strip the symbol table) won't generate backtrace with the line numbers of the actual codes.

BTW, the meiliearch binary in the meilisearch.deb package is stripped.

$ file meilisearch
meilisearch: ELF 64-bit LSB pie executable, x86-64, version 1 (SYSV), dynamically linked, interpreter /lib64/ld-linux-x86-64.so.2, for GNU/Linux 3.2.0, BuildID[sha1]=a814d3cc06512d8134f5488637c5122abbb88d6b, stripped

After finalizing or getting the feedback, I would submit another PR.

@curquiza
Copy link
Member

curquiza commented Oct 20, 2021

Thanks for your quick answer @patrickdung

  1. Not only for performance reasons but also for maintenance, we don't want to provide the musl binary. This might be an addition in the future, but currently it's not something we want 🙂
  2. It's another problem then, so another issue. We don't want to provide stripped binaries. People can strip them on their side if they want be we don't want to encourage this. Thanks for reporting!

@patrickdung
Copy link
Contributor Author

Hello @curquiza

Please note this PR is updated (so there is no need to submit another PR)
Now only aarch64 binary is generated.

Copy link
Member

@curquiza curquiza left a comment

Choose a reason for hiding this comment

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

Since we only have one binary, should we need to use the if condition and the matrix?

.github/workflows/publish-crossbuild.yml Outdated Show resolved Hide resolved
.github/workflows/publish-crossbuild.yml Outdated Show resolved Hide resolved
.github/workflows/publish-crossbuild.yml Outdated Show resolved Hide resolved
patrickdung and others added 3 commits October 21, 2021 17:02
better spacing

Co-authored-by: Clémentine Urquizar <clementine@meilisearch.com>
better naming

Co-authored-by: Clémentine Urquizar <clementine@meilisearch.com>
better naming

Co-authored-by: Clémentine Urquizar <clementine@meilisearch.com>
@patrickdung
Copy link
Contributor Author

For the matrix/if statement, maybe just leave it. It should work, tested in here. If more cross builds is needed, the next people working on it would need to add back those stuff again.

If it needs to remove the "matrix" from the workflow file, it requires several changes and need more time to test. I am busy with other stuff and can't work on it. So, please feel free to update/change it if you want (or by MeiliSearch team).

Copy link
Member

@curquiza curquiza left a comment

Choose a reason for hiding this comment

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

Ok, @patrickdung thanks for the time your spent on this :)

.github/workflows/publish-crossbuild.yml Outdated Show resolved Hide resolved
Update to use the correct syntax

Co-authored-by: Clémentine Urquizar <clementine@meilisearch.com>
Copy link
Member

@curquiza curquiza left a comment

Choose a reason for hiding this comment

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

thanks a lot @patrickdung!

bors merge

If you are participating in Hacktoberfest, and you would like to receive a small gift from MeiliSearch too, please complete this form.

@bors
Copy link
Contributor

bors bot commented Oct 24, 2021

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.

Reducing ArmV8 binary build time with action-rs
2 participants