Use CompositeDecoder::decodeCollectionSize to pre-allocate collections#3192
Use CompositeDecoder::decodeCollectionSize to pre-allocate collections#3192fzhinkin wants to merge 10 commits into
Conversation
fzhinkin
force-pushed
the
fzhinkin/collections-initial-capacity
branch
from
1a41b4d to
c042815
Compare
|
Raw benchmarking results: |
| buffer[position++] = c | ||
| } | ||
|
|
||
| override fun build() = buffer.copyOf(position) |
There was a problem hiding this comment.
To discuss: we can avoid a copy here if position == buffer.size, but we need to explicitly invalidate the builder, JIC.
| val compositeDecoder = decoder.beginStructure(descriptor) | ||
| val expectedSize = compositeDecoder.decodeCollectionSize(descriptor) | ||
| val builder = if (previous != null) { | ||
| previous.toBuilder().also { it.checkCapacity(expectedSize) } |
There was a problem hiding this comment.
This should add the new new "expectedSize" to the current size when calling checkCapacity. The common case for this branch is for incremental parsing of list elements.
It also makes sense to pass the new needed size to toBuilder to avoid an extra copy in the case that a new builder is actually created (not default behaviour).
There was a problem hiding this comment.
Nice catch, thanks for pointing to it!
Not only the code does not work as expected, it can also fail in runtime if checkCapacity can't handle negative expectedSize (which is the case for all non-empty implementations, I guess).
|
Updated benchmarking results: |
Reimplemented
AbstractCollectionSerializer::mergeto read expected collection size first and pass it as an initial capacity to a corresponding builder. Builder were updated to take the capacity into account.The change is quite intrusive, but for formats providing information about the collection size before reading it (it's only CBOR for now, though) it reduces the footprint and slightly improve decoding performance.
Fixes #3153