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
This should save a little memory. We've observed hundreds of such empty arrays
in some applications.
This affects non-empty messages with none of:
- repeated fields
- map fields
- fields to check if they're initialized
PiperOrigin-RevId: 658137927
This may save some alloactions if java's intrinsics aren't smart enough to avoid the roundtrip. But most JVMs probably have smart enough intrinsics, so this is probably not going to speed things up, just make the code look nicer.
PiperOrigin-RevId: 653445330
I believe they have intrinsics even for the long-form Integer.compareTo(Integer.valueOf(a), Integer.valueOf(b)) format which avoids the intermediate allocation.
So this probably won't make things faster, just makes the code a little cleaner.
Integer.compare was added in Java 1.7 which seems safe. Added to Android in SDK 19, which is less than the 21 minSDK supported by protobuf: 303239d74d
PiperOrigin-RevId: 653438420
We are seeing in profiles that this constructor is calling the other constructor.
It doesn't seem to be totally inlined, as the times spent in each constructor
stack frame are different.
In the common case of no builderParent, the constructor can be empty.
PiperOrigin-RevId: 653272368
Protobuf Team Bot
Mike Kruskal
Xufei Tan
Adam Cozzette
Zoey Greer
Mark Hansen
Sandy Zhang
Deanna Garcia