[jira] [Updated] (AVRO-2307) Opt-in setting to improve GC behavior during deserialization?

["Felix GV (JIRA)" <ji...@apache.org>]

     [ https://issues.apache.org/jira/browse/AVRO-2307?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Felix GV updated AVRO-2307:
---------------------------
    Description: 
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 a ButeBuffer.

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. The ButeBuffer 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 a copy.

The caveat in this case is that this only works properly 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 ButeBuffer implementation that throws an exception if any mutation is attempted, indicating that the flag 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.

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 Java Lists, 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.

One challenge here is that the “PrimitiveFloatArray” class is an almost complete copy of the Array class, basically just stripping away the generics. 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. 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 (merey 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 would still provide benefits, however.

—

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.

  was:
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 a ButeBuffer.

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. The ButeBuffer 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 a copy.

The caveat in this case is that this only works properly 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:
 # There could be a config flag on the decoder or elsewhere that allows a user to opt-in to this mode. In this case, it may be safer to return a special read-only ButeBuffer implementation that throws an exception if any mutation is attempted, indicating that the flag ought to be turned off to support mutations.
 # It could be the default mode, but wrapped in a modified ByteBuffer implementation which defers the copy of the content lazily until (and only if) one of the mutation API is called.

Either way, this requires a custom ByteBuffer implementation with special behavior in order to be fully clean and safe, however, in the first approach, the default behavior would still return regular ByteBuffer instances.

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 Java Lists, 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(i) 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.

One challenge here is that the “PrimitiveFloatArray” class is an almost complete copy of the Array class, basically just stripping away the generics. 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. 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 (merey 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 would still provide benefits, however.

—

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.


> 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: Bug
>    Affects Versions: 1.7.7
>            Reporter: Felix GV
>            Priority: Minor
>
> 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 a ButeBuffer.
> 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. The ButeBuffer 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 a copy.
> The caveat in this case is that this only works properly 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 ButeBuffer implementation that throws an exception if any mutation is attempted, indicating that the flag 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.
> 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 Java Lists, 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.
> One challenge here is that the “PrimitiveFloatArray” class is an almost complete copy of the Array class, basically just stripping away the generics. 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. 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 (merey 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 would still provide benefits, however.
> —
> 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)