This brings it into conformance with our spec and other languages. Some parse paths already did this check, and all of them prohibit *nested* unmatched end-group tags.
PiperOrigin-RevId: 705225060
Fixes protocolbuffers#10697: UnsafeDirectNioDecoder modifies shared buffer instance when performing aliased read.
I've included a test-ish reproducer in form of a unit test that fails fairly reliably, let me know whether you'd want this merged.
Closes#19405
COPYBARA_INTEGRATE_REVIEW=https://github.com/protocolbuffers/protobuf/pull/19405 from mernst-github:mernst/aliasing 1d972091c3
PiperOrigin-RevId: 703588369
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
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
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
Protobuf Team Bot
Mike Kruskal
Matthias Ernst
Mark Hansen
cpovirk
nickreid
Protobuf Team Bot
Xufei Tan
Mike Kruskal