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
This is a very narrow edge case where touching a packed extension via generated APIs first, and then doing so reflectively will trigger a DCHECK. Otherwise, reflective APIs will work but not use packed encoding for the extension. This was likely a pre-existing bug dating back to proto3, where it would only be visible on custom options (the only extensions allowed in proto3).
To help qualify this and uncover similar issues, unittest.proto was migrated to editions. This turned up some other minor issues in DebugString and python.
PiperOrigin-RevId: 675785611
Bazel 6 falls back to native rules, because of ProtoInfo differences.
Bazel 7 is slightly degraded: Kythe flags don't work, DebugContext is left out from CcInfo and temporary files generated by the C++ compiler (but it's only useful for debugging).
Tests will be submitted in separate PRs.
PiperOrigin-RevId: 674030212
This avoids allocating a backing array of size 10 when we are adding >10 elements.
- If we are adding objects one at a time, inflate a Object[10] and continue.
- If we are adding objects from a Collection, assume this is the only collection we are adding, and inflate a `Object[collection.size()]`
- If the existing array is non-empty, resize the array by 1.5x exponential growth as usual.
There's another small change where if we're addAll(<10 elements), we allocate an exact-sized backing array (e.g. size=3), rather than rounding up to size 10. See android/LiteAllocationTest. I think this is good: this will save memory in the common case of just calling .addAll() once, and if we call addAll twice, we grow still exponentially. But we could decide to avoid this or split it out into its own change.
This change involves moving some logic out of GeneratedMessageLite. I think the default size of the backing array is better handled inside ProtobufArrayList than inside GeneratedMessageLite's wrapper function.
PiperOrigin-RevId: 673612155
But check that the given byte size is within the bounds of the byte[] first, otherwise we might get OutOfMemoryErrors on bitflipped input that tries to allocate huge arrays.
Add some tests that would have otherwise OutOfMemoryError'd in order to have some confidence that we won't OOM.
PiperOrigin-RevId: 673597245
I'm just about to edit this code, and I want these tests for my peace of mind.
Also update the other callers to use assertThrows for consistency.
PiperOrigin-RevId: 673590917
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.
And in Compiler Explorer, you also see `bufferFixed64NoTag` has reduced from 98 lines of assembly to 57 lines of assembly in the hoisted version. This is because we don't need to re-check the array bounds each time we reload `position`. I imagine this also makes any other method with a fixed number of increments like `writeFixed32NoTag` faster too.
PiperOrigin-RevId: 673588324
This is a no-op cleanup. The methods are package-private so shouldn't be an API break.
I'm about to add some more tests here. This just makes writing the tests easier , as they don't have to catch IOException any more.
PiperOrigin-RevId: 671592242
This ensures that the test for exponential resizing (for amortized linear time)
actually looks exponential.
Previously, we saw in tests that if we add 1000 element-lists 10x in a row, sometimes
we grow the backing array by 6000 bytes twice in a row. (12024 - 18024 - 24024).
Now the test looks much more consistently exponential.
PiperOrigin-RevId: 671577283
Previously, we only make extensions immutable if they were in the FieldSet's
array, maximum size 16. The overflow entries in the TreeMap weren't made
immutable.
PiperOrigin-RevId: 671564444
I will fix this test in the next CL.
This test runs on both `GeneratedMessage` and `GeneratedMessageLite` variants, so we need to first check if `nested instanceof GeneratedMessageLite`, otherwise on the full-variant test, we get `isMutable()` method found on `GeneratedMessage`.
PiperOrigin-RevId: 671563318
This avoids some extra copies and garbage generation.
There's still the extra default 10-sized backing array created by default in
ProtobufArrayList which isn't fixed yet, which we see in allocation tests. That's from `ensureXIsMutable()` which allocates a new list without awareness of the list's size. Maybe we can fix that later: b/362848901.
PiperOrigin-RevId: 670766317
I've tried to keep the hot part of the loop (not null) in the loop without
requiring an extra function call, and only extracted the cold part (null
handling) to avoid repetition.
PiperOrigin-RevId: 670760103
These are very old unit-tests that appear to have been written to provide some conformance capabilities before we had more comprehensive solutions. Today, they should be already covered by our conformance tests and create unnecessary noise.
The goal of this change is to simply desynchronize the Java and C++ text format golden files. Java at least seems to be using these for a dual-purpose for both locking down conformance and unit-testing text format behaviors.
PiperOrigin-RevId: 670596733
These are very old unit-tests that appear to have been written to provide some conformance and fuzzing capabilities before we had more comprehensive solutions. Today, they should be already covered by our conformance tests and fuzz tests and create unnecessary noise.
The goal of this change is to remove the checked-in binary golden data that we no longer know how to regenerate. There is a lot of nearby code that could likely also be cleaned up in a follow-up
PiperOrigin-RevId: 667604310
3.x.x descriptor.proto generated code is *not* supported with 4.x.x runtime, since this results in an ODR violation with the descriptor.proto built into the 4.x.x runtime. This is expected to result in undefined behavior / failures.
Tested against //java/core:v25_generated_message_test_jar (binary compatibility) and //java/core:v25_generated_message_test_srcjar (source compatibility)
PiperOrigin-RevId: 666329342
*** Reason for rollback ***
Rolling back while investigating tsan reports.
*** Original change description ***
Remove a volatile read/write in SmallSortedMap.entrySet()
I don't know why this was volatile.
Volatile semantics are not strong enough for making this read-then-write a
transaction. You'd need a mutex or AtomicReference to achieve that.
***
PiperOrigin-RevId: 662300377
Mark Hansen
Mike Kruskal
Nathan Baulch
Protobuf Team Bot
Mike Kruskal
Xufei Tan
Adam Cozzette
Sandy Zhang