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

benchmarks: main protobuf repo no longer contains benchmark protos #1524

Open
jhump opened this issue Feb 22, 2023 · 2 comments
Open

benchmarks: main protobuf repo no longer contains benchmark protos #1524

jhump opened this issue Feb 22, 2023 · 2 comments

Comments

@jhump
Copy link
Contributor

jhump commented Feb 22, 2023

As of protocolbuffers/protobuf@83c499d, the main protobuf repo no longer contains the benchmark protos used by protocolbuffers-go repo.

When updating to latest version (protocolbuffers/protobuf-go@bc1253a), the benchmark protos had to be left untouched.

The benchmarks for Go either need to be reconciled with the new benchmarks in fleetbench (which is apparently what the main protobuf repo now uses), or it needs to define the benchmark proto sources it needs locally (instead of expecting them to be in the main protobuf repo).

@aktau
Copy link

aktau commented Feb 28, 2023

... the benchmark protos had to be left untouched.

Can you explain this? From what I understand the benchmark protos are (were) on the main protobuf repo. Are you saying this commit should've included a copy of the now deleted benchmark protos?

Also, I recently ran ./regenerate.bash and ./test.bash and didn't see any complaints. How do you run the benchmarks?

@jhump
Copy link
Contributor Author

jhump commented Feb 28, 2023

@aktau, the first link in the description is the commit where they were removed from the main repo. The comment there is that they have been superceded by fleetbench.

The scripts run without complaining because I commented out the code that was expecting them to be in the main repo. That's in the second link -- specifically this part. The benchmarks still successfully run, but they are using an older version of the benchmark protos, the one from Protobuf v21.5 (which is from the last time this repo was updated from a version of the main repo that still included those protos).

So the gist of this issue is that the commented out code should be removed and instead this repo should probably pull benchmark protos from fleetbench and generate Go code for those. And then the actual benchmark tests will need to be updated to reference those protos (which are different than the old benchmark protos that were in the main repo). OR, an alternative, would be to copy those benchmark proto sources into this repo and let this repo be their new home (instead of expecting them to be defined in the main repo).

I hope that makes sense.

gopherbot pushed a commit to protocolbuffers/protobuf-go that referenced this issue Nov 3, 2023
Currently, the script fails, and not with a good error message.

related to golang/protobuf#1524

Change-Id: Ia27a895a7ae2f6349bb1262936e4428fa485bb92
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/538955
Reviewed-by: Damien Neil <dneil@google.com>
Reviewed-by: Lasse Folger <lassefolger@google.com>
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

2 participants