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

Allow pure without purego #3959

Open
zecke opened this issue Jun 12, 2024 · 4 comments
Open

Allow pure without purego #3959

zecke opened this issue Jun 12, 2024 · 4 comments

Comments

@zecke
Copy link
Contributor

zecke commented Jun 12, 2024

What version of rules_go are you using?

0.48.0

What version of gazelle are you using?

0.36.0

What version of Bazel are you using?

bazel 6.4.x

Does this issue reproduce with the latest releases of all the above?

yes

What operating system and processor architecture are you using?

Linux

Any other potentially useful information about your toolchain?

The application is built with --@io_bazel_rules_go//go/config:pure to have no dependencies on GLIBC (or any other c library) as this allows building minimal container images.

What did you do?

Upgraded an application that uses google.golang.org/protobuf to the latest release of rules_go.

What did you expect to see?

No changes in performance.

What did you see instead?

The performance of the application has greatly reduced. Enabling "pure" now implies not using pointer arithmetic (https://github.com/search?q=repo%3Aprotocolbuffers%2Fprotobuf-go%20purego&type=code).

While it seems possible to add gotags in a go_binary it doesn't seem possible to take them away. Using cgo = False will still produce a dynamically linked executable (and require glibc at runtime).

@fmeum
Copy link
Collaborator

fmeum commented Jun 13, 2024

@mattyclarkson I think we (at least I) missed the interaction with unsafe. Rereading the Go thread on this, purego disables:

  • ASM, which may require a C toolchain (not entirely sure about this, does this depend on whether the Go flavor of ASM is used?)
  • unsafe, which doesn't need a C toolchain
  • CGo

We might have to roll back as not having a C++ toolchain doesn't mean you don't want (or can't get) unsafe.

What's the situation for circl? Could we add noasm in addition to purego and only enable the former with pure?

@mattyclarkson
Copy link
Contributor

What's the situation for circl? Could we add noasm in addition to purego and only enable the former with pure?

The upstream change I did for circl was that purego meant exactly that: nothing but pure go.

I didn't even know that there was a Go flavour of asssembly 😊 If that is what is in curve_amd64.s, then the purego tag at the top of that file is incorrect. I can put up a correction to include Go assembly in the build.

We might have to roll back

I'm OK to rollback to fix the performance regression. I feel the fix is upstream though.

not having a C++ toolchain doesn't mean you don't want (or can't get) unsafe.

As far as I understand it, unless a project respects the purego tag and removes files that use unsafe based on that, setting pure = "yes" should not negate using unsafe.

Could we add noasm in addition to purego and only enable the former with pure?

In rules_go we have pure = "{auto,on,off}" and cgo = {True,False}.

This is the current setup for the build flags based on what is enabled in the rules_go target:

rules_go cgo = True cgo = False
pure = "on" purego/cgo purego
`pure = "off" cgo
`pure = "auto" cgo

Note: cgo build flag is implicitly toggled if CGO_ENABLED={0,1}.

Projects could enable C toolchain ASM when both purego / cgo are enabled? Go lang assembly can be included at all times. unsafe can be used at all times.

I'm not a GoLang expert so unsure how these build variant parts interact.

@zecke
Copy link
Contributor Author

zecke commented Jun 14, 2024

@mattyclarkson I think we (at least I) missed the interaction with unsafe. Rereading the Go thread on this, purego disables:

  • ASM, which may require a C toolchain (not entirely sure about this, does this depend on whether the Go flavor of ASM is used?)

I don't think pure ASM will generally require a C toolchain. Let's say I use Go's assembly flavor to implement some code that needs to be "constant time". We will use some of the CPU architecture underlying instructions without having a dependency on external libraries. When it comes to linking Go's internal linker might try to load libraries from the toolchain (e.g. libgcc).

  • unsafe, which doesn't need a C toolchain

Indeed. This is where the performance degradation stems from.

  • CGo

Yes. That directly invokes the compiler and has external dependencies.

We might have to roll back as not having a C++ toolchain doesn't mean you don't want (or can't get) unsafe.

What's the situation for circl? Could we add noasm in addition to purego and only enable the former with pure?

@zecke
Copy link
Contributor Author

zecke commented Jun 14, 2024

he current setup for the build flags based on what is enabled in the rules_go target:

rules_go cgo = True cgo = False
pure = "on" purego/cgo purego
pure = "off" cgo pure = "auto" cgo
Note: cgo build flag is implicitly toggled if CGO_ENABLED={0,1}.

Projects could enable C toolchain ASM when both purego / cgo are enabled? Go lang assembly can be included at all times. unsafe can be used at all times.

When cgo is enabled the resulting binary will have a dependency on the C library. I think adding a row to the table on whether on Linux the resulting go binary has external dependencies would be helpful.

fmeum added a commit that referenced this issue Jun 14, 2024
This reverts commit fcd6390.

`purego` regresses performance of certain third-party libraries (see #3959).
fmeum added a commit that referenced this issue Jun 18, 2024
This reverts commit fcd6390.

`purego` regresses performance of certain third-party libraries (see #3959).
tyler-french pushed a commit that referenced this issue Jun 19, 2024
This reverts commit fcd6390.

`purego` regresses performance of certain third-party libraries (see #3959).
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

No branches or pull requests

3 participants