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

WIP: Generate scala code from protobuf using @com_google_protobuf//:protoc #705

Closed
wants to merge 15 commits into from

Conversation

simuons
Copy link
Collaborator

@simuons simuons commented Mar 1, 2019

Resolves #690

Introduces new rule scala_proto_gen which

  1. generates scala code using implicit @com_google_protobuf//:protoc. This means single version of protoc for proto_library and scalapb_proto_library (instead of one which comes from protoc-jar)
  2. accepts plugin as an executable which implements plugin.proto contract. This means less code for us to supply own scala plugins

scala_proto_srcjar was left intact so it wouldn't break existing users. My suggestion is to deprecate it and remove with subsequent pr.

scala_proto_gen generates code in same way as scala_proto_srcjar does ie all direct and transitive deps are generated into single srcjar (no aspects in this pr)

scalapb_proto_library is implemented on top of scala_proto_gen. One thing I don't like though is that I had to create "private" rule to support JavaInfo in scalapb_proto_library deps (maybe there is a better way to achieve that). I believe scalapb_proto_library deps should accept only proto_library as java_proto_library does.

--proto_path were replaced with --descriptor_set_in this removes burden of constructing -I aliases.

I haven't done benchmarks other than bazel clean followed by bazel test //test/proto/.... I got same times in this pr and master. I don't think this pr has impact on performance. In current implementation protoc process is launched from worker which in turn calls executable plugin and I think in actual code generation phase workers are not used (I might be wrong here).

@simuons simuons requested a review from johnynek as a code owner March 1, 2019 08:39
@simuons
Copy link
Collaborator Author

simuons commented Mar 1, 2019

BTW I'm going to test this on our codebase. I should have done it before opening PR but better later than never I guess.

@johnynek
Copy link
Member

johnynek commented Mar 1, 2019

Looks like we have two racing proto rewrites. Can you and @ianoc-stripe look at each other’s changes and comment on what aspects from each should be used?

@johnynek
Copy link
Member

johnynek commented Mar 1, 2019

See #700

Copy link
Member

@johnynek johnynek left a comment

Choose a reason for hiding this comment

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

I’m starting to wonder if we should have two separate files for two different implementations if there are trade offs here.

Like unless I am wrong this is trading performance for ease of integration with protoc. For us, and I think many, we don’t want to trade performance for ease of configuration, we’d probably just want to do the extra config work.

inputs = [ctx.executable._protoc, ctx.executable.plugin] + descriptors,
outputs = [srcdotjar],
arguments = [
"--plugin=protoc-gen-scala=" + ctx.executable.plugin.path,
Copy link
Member

Choose a reason for hiding this comment

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

This is going to prevent using workers right? So you have to spin up a new JVM for each file, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes it would prevent using workers. I thought this is what is happening now as well. Now I think my assumptions might be wrong as protoc-bridge is doing some tricks to reuse jvm. I'll dig more and update.

@ittaiz
Copy link
Member

ittaiz commented Mar 1, 2019

@johnynek I think @simuons needs to upload a diagram of our (rules_scala) current process map when using proto.
He explained it to me and AFAIU this will be a net performance improvement.
I do however agree that we need to see how we have both rewrites.
@simuons I think you should consider yielding to @ianoc since he was first unless you think yours greatly simplifies his (which I don't see but might be wrong).

@simuons
Copy link
Collaborator Author

simuons commented Mar 3, 2019

@ittaiz @johnynek I didn't mean to race/interfere with aspects PR. I'm trying to solve different issue here (ie use single @com_google_protobuf//:protoc) which is orthogonal to aspects. I'd say aspects PR should move forward and ignore this one. I still need to investigate performance impact.

PS thanks for your time and sorry for delayed response.

@simuons simuons changed the title Generate scala code from protobuf using @com_google_protobuf//:protoc WIP: Generate scala code from protobuf using @com_google_protobuf//:protoc Mar 3, 2019
@ittaiz
Copy link
Member

ittaiz commented Mar 3, 2019 via email

@simuons
Copy link
Collaborator Author

simuons commented Mar 4, 2019

Apparently my assumptions about worker usage when generating scala code were wrong. Code generation indeed happens in worker by the trick of protoc-bridge. protoc is supplied with a plugin that pipes it's inputs/outputs to files https://github.com/scalapb/protoc-bridge/blob/master/src/main/scala/protocbridge/frontend/PosixPluginFrontend.scala#L50 and worker reads/writes to them.

Closing this in favour of less aggressive attempt to reuse @com_google_protobuf//:protoc #708

@simuons simuons closed this Mar 4, 2019
@lberki
Copy link
Contributor

lberki commented Mar 11, 2019

@ittaiz : iff something goes through java_common.compile, it should use workers. AFAIU the Scala plugin is written in Scala, so I suppose then no workers will be used? (modulo, of course, protoc-bridge, which I don't really understand...)

@simuons simuons deleted the scala-proto-gen branch October 23, 2020 08:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants