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

Use shared linkage for benchmarks #1336

Merged
merged 1 commit into from
Oct 26, 2023
Merged

Use shared linkage for benchmarks #1336

merged 1 commit into from
Oct 26, 2023

Conversation

fhanau
Copy link
Collaborator

@fhanau fhanau commented Oct 24, 2023

We already use shared linkage for tests – cc_test uses shared linkage by default. This allows us to compile benchmark binaries for CI debug/ASAN builds, which would otherwise be prohibitively large.

@fhanau fhanau requested review from mikea and ohodson October 24, 2023 13:43
@fhanau
Copy link
Collaborator Author

fhanau commented Oct 24, 2023

@mikea Any concerns about shared linkage affecting benchmark performance? If it would make benchmarking less accurate we could disable it in the benchmark/profile configurations.

@ohodson
Copy link
Contributor

ohodson commented Oct 24, 2023

I just noticed nodejs download failureus in the tests, looks like a bad rebase on my side. Fix coming.

@ohodson
Copy link
Contributor

ohodson commented Oct 24, 2023

I just noticed nodejs download failureus in the tests, looks like a bad rebase on my side. Fix coming.

#1337

@fhanau
Copy link
Collaborator Author

fhanau commented Oct 24, 2023

I just noticed nodejs download failureus in the tests, looks like a bad rebase on my side. Fix coming.

#1337

Thank you! Merged it so this can be unblocked.

@fhanau
Copy link
Collaborator Author

fhanau commented Oct 24, 2023

Looks like this doesn't work on Windows 🫤
I've seen issues with shared linkage there before, might keep this disabled for a bit still, might have to only enable this on Linux only and keep the benchmarks disabled on other platforms for now.

We already use shared linkage for tests – cc_test uses shared linkage by
default. This allows us to compile benchmark binaries for CI debug/ASAN
builds, which would otherwise be prohibitively large. For now, this is
only enabled on Linux.
@fhanau
Copy link
Collaborator Author

fhanau commented Oct 26, 2023

Investigated what it would take to support shared linkage on Windows – apparently global variables that should be visible across libraries need to be specifically annotated as __declspec(dllimport), so bazel's approach of allowing any library to implicitly be shared won't work without extensive code changes. Only having this on Linux is not ideal, but at least we'll have them compiled with asan/debug somewhere.

@fhanau fhanau merged commit e01f8a7 into main Oct 26, 2023
10 checks passed
@fhanau fhanau deleted the felix/shared-bench branch October 26, 2023 18:36
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.

3 participants