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

Add Docker image for cargo-pgo #52

Merged
merged 6 commits into from
Apr 3, 2024
Merged

Add Docker image for cargo-pgo #52

merged 6 commits into from
Apr 3, 2024

Conversation

bioinformatist
Copy link
Contributor

@bioinformatist bioinformatist commented Apr 1, 2024

Hey Jakub! Sorry for such a late PR!

To deploy an image with your Docker Hub account, you may need to generate an access token and then add your account and token to this repo's secrets.

I've tested the new workflow with a test-use repository and it works well. These repositories will be deleted once this PR is merged 😄

@Kobzol
Copy link
Owner

Kobzol commented Apr 2, 2024

Hi, thanks a lot for the PR!

Regarding the LLVM installation approach, have you tried that BOLT indeed works, both for instrumentation and then for generating samples? I haven't tried with LLVM 18, but with previous versions, the installed bolt-XY package from Ubuntu haven't been functioning properly (#31). It was detected, but failed during instrumentation/sample gathering.

@bioinformatist
Copy link
Contributor Author

bioinformatist commented Apr 2, 2024

Yeah the BOLT also works well. You can test it locally using cargo-pgo itself in these steps:

Firstly, build the image and get into the container:

git clone https://github.com/bioinformatist/cargo-pgo.git && cd cargo-pgo
docker build -t pgo .
docker run --entrypoint bash -v $(pwd):/workdir --rm -it pgo

Then in the container:

cargo pgo build
/workdir/target/x86_64-unknown-linux-gnu/release/cargo-pgo pgo info
cargo pgo bolt build --with-pgo
/workdir/target/x86_64-unknown-linux-gnu/release/cargo-pgo-bolt-instrumented pgo info
cargo pgo bolt optimize --with-pgo

Below shows the result:

image

I believe the issue lies in the suffix of the binaries (like the -18 in merge-fdata-18) and the incorrect location of the linking library (.a), which caused the bolt-XY package from Ubuntu to not work. The soft linking (ln -s) in Dockerfile can fix it. While compiling LLVM from the source code is a more elegant way, it spends more time, which maybe not ideal for CI.

Copy link
Owner

@Kobzol Kobzol left a comment

Choose a reason for hiding this comment

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

Ah, I didn't notice the symlinks. That's great, thanks! I left a few comments, mostly I want to simplify the PR a bit.

.github/workflows/publish.yml Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@bioinformatist
Copy link
Contributor Author

Got that! I'll finish it ASAP.

@Kobzol
Copy link
Owner

Kobzol commented Apr 2, 2024

No need to hurry ;)

Then run this in your project directory to create a container:

```bash
docker run -v $(pwd):/workdir --rm -it cargo-pgo
Copy link
Owner

Choose a reason for hiding this comment

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

The resulting files will be owned by root by default 🤔 Maybe we should somehow mention setting a different user ID. But it's probably outside of scope for this README.

Copy link
Owner

@Kobzol Kobzol left a comment

Choose a reason for hiding this comment

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

Thank you! :)

@Kobzol Kobzol enabled auto-merge (rebase) April 3, 2024 12:16
@Kobzol Kobzol merged commit d18f374 into Kobzol:main Apr 3, 2024
5 checks passed
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