The proto
message A {
SomeMessage text_field = 1;
SomeMessage text = 2;
}
will fail java compilation with a name clash: `method getTextFieldBuilder() is already defined in class...` This is because the `text` field creates a private method (`get{Text}{FieldBuilder}`) which conflicts with the public getter (`get{TextField}{Builder}`) for field 1.
Unlike some name clashes, this one is with a private method and we can just rename the method in this file. Other name clash issues:
- https://github.com/protocolbuffers/protobuf/issues/15411
- https://github.com/protocolbuffers/protobuf/issues/17367
There's some precedent for the `internal` prefix with the protected `internalGetFieldAccessorTable` method on GeneratedMessage.
PiperOrigin-RevId: 694108040
Technically these methods fail either if the field is primitive (non-message) or if the field is repeated. However it is confusing to have a repeated message field claim to be of type 'message' and then fail with an error message that claims you need a message type.
PiperOrigin-RevId: 693789370
For most _native_ loads of Java rules (e.g. java_library), these can be automatically handled by Bazel 8's autoload feature.
However, this is not the case when the main repository (IOW, the repository currently being built) is a rule repo itself. In those cases, module dependencies are not automatically loaded.
This PR fixes a compatibility issue of OSS protobuf Bazel rules with other Bazel rule repositories such as bazelbuild/rules_android.
Without this PR, builds within rules repositories with Bazel 8 will fail like so:
```
external/protobuf+/java/core/BUILD.bazel:142:13: @@protobuf+//java/core:lite: no such attribute 'srcs' in 'java_library' rule
```
PiperOrigin-RevId: 690735533
The JVM regex engine allocates garbage on every match (especially when calling Matcher.usePattern!). Since there are expected to be a lot of tokens, this caused substantial GC overhead.
Direct char scanning also opens the possibility of other optimizations that aren't possible with regexes. For example:
- direct reads from a char[]
- streaming tokenization (rather than reading the complete source text)
PiperOrigin-RevId: 689230675
It's not used by generated code, contrary to the comment.
The 2-arg version of the method was used in gencode for one day (2025-04-08 through 09): cl/90574926 to cl/90648831.
PiperOrigin-RevId: 688268752
The default name for the rules_jvm_external.maven rule is "maven". When not set, it defaults to "maven". For root modules also using rules_jvm_external, the name clash causes a warning:
```
DEBUG: $TMP/external/rules_jvm_external~/private/extensions/maven.bzl:154:14:
The maven repository 'maven' is used in two different bazel modules,
originally in '<my_workspace>' and now in 'protobuf'
```
Summarizing @shs96c in [1]:
> The common maven repo name allows rulesets to contribute to the user's JARs.
> However, this implies that maven is for the end user, not for transitive
> dependencies. If a ruleset needs private dependencies, it should use a custom
> namespace rather than the maven namespace.
Since protobuf is not contributing to user's JARs, we'll use a custom namespace. There's precedent for using a custom namespace for library modules:
- rules_jvm_external uses `rules_jvm_external_deps` instead of `maven`.
- rules_kotlin uses `kotlin_rules_maven` instead of `maven`.
[1]: https://github.com/bazel-contrib/rules_jvm_external/issues/916#issuecomment-1645527584Fixes#16839.
Closes#18641
COPYBARA_INTEGRATE_REVIEW=https://github.com/protocolbuffers/protobuf/pull/18641 from jschaf:joe/protobuf-maven bd2c62f311
PiperOrigin-RevId: 684625084
If the first check passed (we're an ArrayList) then we aren't a ProtobufArrayList, and there's no point checking.
Micro-optimisation.
PiperOrigin-RevId: 683793700
I think all these generics only change things at compile-time, not at runtime. So should be safe.
It's not a huge win; there's still some unchecked casts. But I was able to remove some unchecked casts, and some warning suppressions, so it's incrementally better.
PiperOrigin-RevId: 683793374
See godbolt for Android ART compiler: https://godbolt.org/z/M9dWhdqbf
This optimisation brings the implementation down from 284 bytes to 272 bytes.
- `GeneratedMessage$Builder SingleFieldBuilder.getBuilder() [284 bytes]`
- `GeneratedMessage$Builder SingleFieldBuilder.getBuilder__withLocalVariable() [272 bytes]`
It's not big. It's just a few instructions dropped. These were dropped just before the `ret`:
```
-mov x23, x1
-ldr w0, [x23, #8]
```
And this load dropped before calling `markClean`.
```
-ldr w1, [x23, #8]
```
PiperOrigin-RevId: 683619150
https://docs.oracle.com/javase/specs/jls/se10/html/jls-5.html#jls-5.1.3
The JLS guarantees this is the same:
> A narrowing conversion of a signed integer to an integral type T simply discards all but the n lowest order bits, where n is the number of bits used to represent type T. In addition to a possible loss of information about the magnitude of the numeric value, this may cause the sign of the resulting value to differ from the sign of the input value.
PiperOrigin-RevId: 683416604
https://docs.oracle.com/javase/specs/jls/se10/html/jls-5.html#jls-5.1.3
The JLS guarantees this is the same:
> A narrowing conversion of a signed integer to an integral type T simply discards all but the n lowest order bits, where n is the number of bits used to represent type T. In addition to a possible loss of information about the magnitude of the numeric value, this may cause the sign of the resulting value to differ from the sign of the input value.
In practice, Android's optimising compiler saw that this was unnecessary, and skipped generating 'and' instructions. So this isn't a performance boost, just a cleanup.
PiperOrigin-RevId: 683395327
https://docs.oracle.com/javase/specs/jls/se10/html/jls-5.html#jls-5.1.3
The JLS guarantees this is the same:
> A narrowing conversion of a signed integer to an integral type T simply discards all but the n lowest order bits, where n is the number of bits used to represent type T. In addition to a possible loss of information about the magnitude of the numeric value, this may cause the sign of the resulting value to differ from the sign of the input value.
In practice, Android's optimising compiler saw that this was unnecessary, and skipped generating 'and' instructions. So this isn't a performance boost, just a cleanup.
PiperOrigin-RevId: 683389646
Previously, we didn't test for OutOfSpaceException, because all writes were buffered. The OutOfSpaceException wouldn't happen until flushing.
If we flush the stream, we can detect OutOfSpaceException.
Further, I saw some tests saying "If streaming, try different block sizes." These tests used block size (1, 2, 4, 8, 16), which are all less than the minimum block size of 20. So all these were rounded up to 20, and were not doing much useful: http://google3/third_party/java_src/protobuf/current/java/com/google/protobuf/CodedOutputStream.java;l=2280;rcl=679381814.
I've replaced such loops with a two OutputTypes:
- STREAM, which has a buffer of the size of the output
- STREAM_MINIMUM_BUFFER_SIZE, which has a buffer size 20
This allows deleting some extra duplicated code.
PiperOrigin-RevId: 681756512
This produces much better code for Android: https://godbolt.org/z/xE8T9xqrr
Down from 196 bytes to 140 bytes. Bounds checks get combined together.
This is a partial roll-forward of cl/673588324.
PiperOrigin-RevId: 681703327
This saves one ARM instruction (`mov x1, x4`) when the array is out of bounds:
https://godbolt.org/z/7Gb7so4Ez
Because the side effects of position++ have to happen even if the array overflows.
It's fairly minor. Probably won't make a big difference.
This is a partial roll forward of cl/673588324.
PiperOrigin-RevId: 681696805
Other methods are now updating position at the end of the method.
Also, it's good practice to limit the scope of try { } blocks.
This is a partial roll-forward of cl/673588324
PiperOrigin-RevId: 681673291
When writing varints.
This has twofold goals:
1. Correctness: if position overruns the array, checking space left may return a negative number. I'm not sure how bad that is, but let's avoid it.
2. Performance. This generates more optimal assembly code which can combine bounds checks, particularly on Android (I haven't looked at the generated assembly on the server JVM; it's possible the server JVM can already performance this hoist).
The `position` field is stored on the object, so Android ART generates assembly codes for `this.position++` like "load, add, store":
```
ldr w3, [x1, #12]
add w4, w3, #0x1 (1)
str w4, [x1, #12]
```
There can be a lot of these loads/stores executed each step of a loop (e.g. writeFixed64NoTag updates position 8 times, and varint encoding could do it even more). It's faster if we can hoist these so we load once at the start of the function, and store once at the end of the function. This also has the nice benefit that it won't store if we've thrown an exception.
See before/after in Compiler Explorer: https://godbolt.org/z/bWWYqsxK4. I'm not an assembly expert, but it seems clear that the increment instructions like `add w4, w0, #0x1 (1)` are no longer always surrounded by loads and stores in the new version.
PiperOrigin-RevId: 681644516
Before, some encoders would not give any details about position/limit/length.
Now a few more places do.
Just found this while trying to add some tests for the exception message, and
found some encoders weren't setting it.
This doesn't fix all the places that OutOfSpaceException didn't have a useful message.
PiperOrigin-RevId: 681218740
If this code is in every method, it's making every method larger, putting
pressure on the instruction cache.
The string formatting is an exceptional case that shouldn't slow down the
regular case.
This should modestly speed up the code in here. And besides, it's just a little
nicer to have this formatting centralised.
PiperOrigin-RevId: 679336304
On Android, this generates better assembly code, bounds-checking through all
the used indices upfront, and branching to deoptimise if it's not true,
avoiding doing 4x bounds checks. We also don't generate 4 different
`pThrowArrayBounds` code sections.
https://godbolt.org/z/Kbhvcdvbd
Code size Comparison:
- `void X.writeFixed32NoTag__before(int) [292 bytes]`
- `void X.writeFixed32NoTag__after(int) [180 bytes]`
This starts by throwing a more meaningful length (4bytes or 8bytes for fixed64), which makes sure the value of position in the catch clause isn't dependent on which line threw the exception.
PiperOrigin-RevId: 678543462
This was untested before.
Some of the test names had drifted from the code's names; update those.
These tests have surfaced some problems around inconsistent exception types thrown.
PiperOrigin-RevId: 677588477
These tests were (generally) looping over OutputType already. Some tests were looping over a subset; I've expanded many tests to loop over more OutputTypes.
But the first failure they encountered with any OutputType meant they'd halt
that test, without testing the other OutputTypes. That's frustrating.
We use `assume()` to discard tests in the matrix that are irrelevant.
There are many java parameterized test runners. I followed the lead of
third_party/java_src/protobuf/current/javatests/com/google/protobuf/IsValidUtf8FourByteTest.java,
which uses Paramaterized runner.
This means:
- We see which output type is failing in the test name.
- We don't have to always assertWithMessage(OutputType.name()). We can just use
assertThat. Nice.
- It's really easy to add new coders, and run all the tests against them. I've
done that here for NIO encoders with offset, increasing their test coverage.
PiperOrigin-RevId: 677564209
This makes it clearer that we get the arguments around the right way, so the error messages make sense.
Also get rid of toList; it doesn't seem necessary, the Truth library can compare byte[]s just fine without first turning them into List<Byte>.
This is just a refactoring to make this test nicer before I add more tests covering out of space scenarios.
PiperOrigin-RevId: 676963220
cpovirk
Protobuf Team Bot
nickreid
Ted Xie
Mark Hansen
Protobuf Team Bot
Joe Schafer
Xufei Tan
Mike Kruskal
Nathan Baulch