[
https://issues.apache.org/jira/browse/AVRO-2307?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16820731#comment-16820731
]
Felix GV commented on AVRO-2307:
--------------------------------
FYI, the following project contains a bunch of performance optimizations
(including #2 above) as well as more robust compatibility across versions of
Avro: https://github.com/linkedin/avro-util
> Opt-in setting to improve GC behavior during deserialization?
> -------------------------------------------------------------
>
> Key: AVRO-2307
> URL: https://issues.apache.org/jira/browse/AVRO-2307
> Project: Apache Avro
> Issue Type: Improvement
> Components: compatibility, java
> Affects Versions: 1.7.7
> Reporter: Felix GV
> Priority: Minor
> Labels: performance
>
> We have a performance-sensitive project that leverages Avro for an online
> data store, and I wanted to report on a couple of Avro deserialization
> optimizations we have implemented. On one hand, it is great that Avro’s code
> is clean and modular enough to have allowed us to achieve this easily. But on
> the other hand, we are leveraging parts of the API which are probably not
> typically used by most users, and thus we are exposing ourselves to ongoing
> maintenance costs as those “ambiguously-public” APIs might change in future
> versions. For this reason, I wanted to gauge the appetite of the Avro
> community for taking in those optimizations upstream into the main project.
> The minor challenge is that the optimizations we’ve made are not completely
> invisible, and therefore should probably be presented as opt-in settings,
> rather than new defaults. Below is a summary of both changes.
> *1. Re-use of byte arrays when instantiating ByteBuffers*
> When deserializing a byte array that contains a ByteBuffer field, the
> relevant portion of the input byte array is copied into a new byte array,
> which is then used as the backing array of a new ByteBuffer. This is
> completely safe, but appears to be inefficient from our point of view,
> requiring extra CPU cycles and generating garbage.
> In our case, we have a few schemas which contain some general metadata and an
> opaque byte array payload, which often ends up being a significant portion of
> the total byte length. Recopying these bytes results in up to 2x the byte
> allocation (assuming the schema contained just a ByteBuffer field and nothing
> else). The ByteBuffer API, however, provides an alternative behavior where
> the backing array can be larger than needed, with an offset and length
> provided to indicate the internal boundaries of the payload. In our
> implementation, we re-use the input byte array as the ButeBuffer’s backing
> array, therefore avoiding both the allocation and the copy.
> The caveat in this case is that this only works safely for use cases that
> don’t mutate the content of the bytes (neither the input nor the deserialized
> object). In our case this assumption is valid.
> If this was implemented in the open-source project, there are a few ways this
> could be achieved:
> # We could return a special read-only ByteBuffer implementation that throws
> an exception if any mutation is attempted, indicating that this special mode
> ought to be turned off to support mutations.
> # We could return a modified ByteBuffer implementation which defers the copy
> of the content lazily until (and only if) one of the mutation API is called.
> More user-friendly, perhaps, but could introduce hidden overhead which the
> application designer might prefer to be alerted to rather than being silently
> shoved under the rug.
> Either way, this probably requires a custom ByteBuffer implementation with
> special behavior in order to be cleaner. In both cases, however, I don’t see
> a way to guard against mutations to the input byte array, therefore, this
> should probably be an opt-in setting for users who know what they’re doing
> and are comfortable with the read-only assumption in their workloads.
> *2. Primitive (non-boxing) implementation of lists*
> Another challenge we’ve come across is that lists of primitive types (floats
> in our case) are always boxed into Object Floats by Avro. In our case, this
> resulted in millions of Objects / second getting allocated, causing
> pathological GC pressure.
> To solve this, we have implemented an alternative version of the Avro Array
> class, but which instead of hanging on to an array of generically-typed
> Objects, internally, hangs on to an array of primitive floats. This causes no
> boxing at deserialization time, but there is a further challenge which is
> that since Avro array fields are exposed as instances of the Java List
> interface, the regular functions of the API all return Objects, therefore
> merely deferring the boxing to a slightly later point in time. To get around
> this further complication, we have added a getPrimitive(index) function which
> returns primitive items directly. In order to be able to use this more
> optimized function, it is necessary for us to cast the list into our own
> type, otherwise we wouldn’t see the new function. The end result is quite
> dramatic, performance-wise, reducing our p99 latencies down to a quarter to a
> third of their original values, depending on workloads.
> One challenge here is that the “PrimitiveFloatArray” class is an almost
> complete copy of Avro’s Array class, basically just stripping away the
> generics and adding the primitive-returning function. If we were to
> contribute this upstream to the open-source project, I imagine we might want
> to do this not only for floats but for boolean, int, long and double arrays
> as well. This would mean roughly 5x the same copy-pasted implementation,
> which is not ideal from a maintenance standpoint. The generic types are nicer
> in that sense, but unfortunately, Java generics do not support primitives
> (AFAIK, unless one of the latest Java releases introduced support for this
> recently?). In our case, we are willing to pay that maintenance cost in
> exchange for the dramatic GC reduction it gives us, but I don’t know if the
> Avro project would come to the same conclusion.
> If implemented, this could potentially be the new default behavior, though
> boxing would still happen (merely deferred) unless users casted the lists
> into the new types and used the new primitive-returning functions. In use
> cases where an entire primitive array is deserialized but only a subset of
> its items are accessed, this boxing deferral could still provide benefits,
> however, even without code changes. I imagine there might be a counter case
> though where a primitive array gets deserialized and then fully accessed
> multiple times, in which case deferred on-demand boxing might be even worse...
> —
> So again, I re-iterate: the goal of this ticket is to share what we’ve found
> and what we’ve done about it, with the intent to understand if there would be
> interest in reviewing and merging a patch which provided the above opt-in
> benefits.
> Thanks for your feedback.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
