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
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.
Maybe it's so that if someone else has already made a EntrySet on another
thread, we can reuse that? But, it's probably cheaper to make a new EntrySet
probably than to do the volatile read-or-write (this is just a guess, may be wrong, I'm
not super experienced with the cost of volatiles).
The EntrySet is stateless (except for the state of its containing
SmallSortedMap) so it's fine to have more than one of them.
The commit introducing this doesn't explain why it's volatile, but the method
comment referring to "Similar to the AbstractMap implementation of {@code
keySet()}" may have some clues.
AbstractMap.keySet uses a transient keySet, not a volatile keySet.
Is it possible that this initial implementation mistook transient for volatile?
Anyway, it's unnecessary, let's rip it out and enjoy a little more performance.
PiperOrigin-RevId: 662071950
I've noted the rationale for this in the comment, and in the attached bug.
This should save about 4-8 bytes of memory (on the 64bit server JVM depending on if compressed object pointers are on) per object with extensions. And 4 bytes on Android.
PiperOrigin-RevId: 661900014
We mostly use generated pom files in our release currently, so we can delete all the files that aren't used and the tools to update them.
Note, java/bom/pom.xml java/pom.xml and java/protoc/pom.xml are all still used at release and java/kotlin/pom.xml is used for documentation so all of those need to stay for now.
PiperOrigin-RevId: 659664012
Store them as Object[], because you can't have a Entry[], because Entry[] has a generic 'this' over <K, V>, you get error "generic array creation" if you try to make "new Entry[]".
And if you try to cast Object[] to Entry[], you get ClassCastException. So I've just made it an object array that we cast when reading out of.
This should save memory and improve performance, because we have fewer pointers to chase.
Also fixed some warnings about unnecessary unchecked suppressions, and type-name should end with T.
PiperOrigin-RevId: 658585983
Mike Kruskal
Mark Hansen
Nathan Baulch
Protobuf Team Bot
Mike Kruskal
Xufei Tan
Adam Cozzette
Sandy Zhang
Deanna Garcia