google / auto

A collection of source code generators for Java.
Apache License 2.0
10.43k stars 1.2k forks source link

Builders without autoboxing #456

Closed bubenheimer closed 7 years ago

bubenheimer commented 7 years ago

I would like to have generated AutoValue Builders that do not autobox primitive values for purposes of a null check, but instead use separate boolean fields. I am concerned about the use case of low-end Android devices and extensive Builder-based creation of short-lived objects. I'd think that this could cause problems under the right circumstances, as auto-boxed objects are created only to be discarded right away. Likely a more pressing issue on Dalvik than ART, but might cause a problem no matter what. I have not benchmarked it, but am speaking from general past experience with extensive creation of short-lived small objects in Java.

The current design simply seems to make the wrong trade-off. Builders are typically short-lived and relatively small objects, so it would not commonly matter if we make them close to twice as large in the worst case with additional boolean fields as long as it's just one single Builder object. By contrast, it seems needlessly wasteful to create many additional objects with each Builder.

In fact, I assume that the size of the object overhead of the autoboxed primitives would generally outweigh the memory overhead of the extra boolean.

Ironically the property-heavy use case, where the builder pattern is the most appropriate, is also the one that currently creates the most object garbage. For an object with 10 primitive non-null properties:

eamonnmcmanus commented 7 years ago

That's an interesting point. In fact, earlier implementations of builders used a single java.util.BitSet to track which properties had been set. We changed this because the code is much simpler using null to track, and more importantly because java.util.BitSet is not supported on GWT. Conceivably we could use the bits of an int to track, when there are no more than 32 required properties, and long when there are no more than 64, and say that we don't support having more than 64 required properties on GWT. That's a lot of properties, after all. I think we'd want some more concrete evidence that the current scheme is problematic before embarking on that change, though.

bubenheimer commented 7 years ago

When compared to the current solution taking the simple route of using boolean fields would be both relatively easy to achieve and more performant with smaller memory footprint due to no object overhead. The generated code would still be very readable, too. Embarking on the more complex solution that you envision may be better served by creating a separate Issue.

JakeWharton commented 7 years ago

Using booleans are spectacularly wasteful for anything but a single-property builder as they rapidly inflate the size of the builder object in memory compared to using bits. Using the JOL tool can confirm this.

Have you actually measured this to be a problem on Android? I'm skeptical of it even being a problem. Not only are the object short-lived, but common values likely fall withing the cached instances inside the boxed types that won't actually allocate. Of course this is also just speculation as the original issue.

On Sun, Mar 19, 2017, 3:54 PM Uli Bubenheimer notifications@github.com wrote:

When compared to the current solution taking the simple route of using boolean fields would be both relatively easy to achieve and more performant with smaller memory footprint due to no object overhead. The generated code would still be very readable, too. Embarking on the more complex solution that you envision may be better served by creating a separate Issue.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/google/auto/issues/456#issuecomment-287642596, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEEEZSSgRsLYCeLyiK0qmZUIk_e78Oyks5rnYfkgaJpZM4MhcQx .

bubenheimer commented 7 years ago

I am not against using bit arrays, yet from @eamonnmcmanus's response it seems that there may be more hurdles to overcome to see them come about, in spite of their technical superiority. I see benefits in using booleans over boxing primitives, which may be easier to bring about. If we end up with bit arrays, even better.

The degree of actual benefit would depend on the JVM and the specific use case. I am not intimately familiar with all the intricacies of ART or Dalvik, but I generally consider an approach with fewer objects more robust and preferable. Some years ago I was running Java on IBM mainframes and ran into problems with creating large numbers of short-lived objects that were so severe that we had to switch to an approach based on arrays of primitives. The GC could not keep up despite maximum tuning. I was not the only one with that kind of problem. I assume a similar issue also exists on Android, at least on Dalvik, which is still in relatively wide use.

eamonnmcmanus commented 7 years ago

I think this issue is a bit too speculative to be actionable. We might undertake the non-trivial task of rewriting to use bit sets if there are performance measurements supporting that. It's also possible that that implementation will prove more convenient as AutoValue evolves. But I don't think it's useful to leave this issue open in the meantime.