Remove {direct,transitive}OriginalSources field from ProtoInfo #12727
Conversation
| @Deprecated | ||
| public static ProtoLangToolchainProvider create( | ||
| String commandLine, | ||
| FilesToRunProvider pluginExecutable, | ||
| TransitiveInfoCollection runtime, | ||
| NestedSet<Artifact> blacklistedProtos) { | ||
| return new AutoValue_ProtoLangToolchainProvider( | ||
| commandLine, pluginExecutable, runtime, blacklistedProtos); | ||
| commandLine, pluginExecutable, runtime, ImmutableList.of(), blacklistedProtos); | ||
| } |
There was a problem hiding this comment.
I checked internally and the method may be safely deleted. And then I believe you can use NestedSet for providedProtoSources.
| @Deprecated | ||
| public abstract NestedSet<Artifact> blacklistedProtos(); | ||
|
|
There was a problem hiding this comment.
Keep. Can't be safely removed.
| // We intentionally flatten the NestedSet here. | ||
| // | ||
| // `providedProtoSources` are read during analysis, so flattening the set here once | ||
| // avoid flattening it in every `<lang>_proto_aspect` applied to `proto_library` | ||
| // targets. While this has the potential to use more memory than using a NestedSet, | ||
| // there are only a few `proto_lang_toolchain` targets in every build, so the impact | ||
| // on memory consumption should be neglectable. | ||
| providedProtoSources.build().toList())) |
There was a problem hiding this comment.
Why do you need to flatten the NestedSet? If I understand this correctly, so that you can use overloaded ProtoLangToolchainProvider.create? I'd prefer if you remove the old implementation of create and just use NestedSet. I checked that there are no dependencies internally. If you're worried about other forks of Bazel, perhaps you could change the create method name?
There was a problem hiding this comment.
No, this not for overloading ProtoLangToolchainProvider#create(). Today, every <lang>_proto_library has to flatten the set to figure out which of its srcs is in blacklistedProtos(), which does have performance overhead. We can avoid that by flattening it here once and propagate the excluded protos as list instead. We trade some (finite) memory for performance.
The information is already part of another field. Cleanup after bazelbuild#12431 and bazelbuild#10939
Yannic
force-pushed
the
proto_remove_orig_p1
branch
from
0739386 to
5a3dc64
Compare
| // We intentionally flatten the NestedSet here. | ||
| // | ||
| // `providedProtoSources` are read during analysis, so flattening the set here once | ||
| // avoid flattening it in every `<lang>_proto_aspect` applied to `proto_library` | ||
| // targets. While this has the potential to use more memory than using a NestedSet, | ||
| // there are only a few `proto_lang_toolchain` targets in every build, so the impact | ||
| // on memory consumption should be neglectable. | ||
| providedProtoSources.build().toList())) |
There was a problem hiding this comment.
No, this not for overloading ProtoLangToolchainProvider#create(). Today, every <lang>_proto_library has to flatten the set to figure out which of its srcs is in blacklistedProtos(), which does have performance overhead. We can avoid that by flattening it here once and propagate the excluded protos as list instead. We trade some (finite) memory for performance.
| @Deprecated | ||
| public static ProtoLangToolchainProvider create( | ||
| String commandLine, | ||
| FilesToRunProvider pluginExecutable, | ||
| TransitiveInfoCollection runtime, | ||
| NestedSet<Artifact> blacklistedProtos) { | ||
| return new AutoValue_ProtoLangToolchainProvider( | ||
| commandLine, pluginExecutable, runtime, blacklistedProtos); | ||
| commandLine, pluginExecutable, runtime, ImmutableList.of(), blacklistedProtos); | ||
| } |
The information is already part of another field.
Cleanup after #12431 and #10939