commit 3972356c2bad8e77adbec04f735246c91dc7024d
author Yigit Boyar <yboyar@google.com> Mon Oct 04 15:08:42 2021 -0700
committer Yigit Boyar <yboyar@google.com> Tue Oct 12 11:24:26 2021 -0700
tree 118447b5be4a39cdc74c9e6354d329919eaa4a41
parent 9cd4959c836319c3f047f1bc776fbc5b0b21d5c9

Add alternative TypeConverterStore for KSP

This CL introduces a new TypeConverterStore implementation that uses a
more fine granular cost calculation.

The current TypeConverterStore has a constant cost for each type
converter. Moreover, it does not optimize for nullability, resulting in
Room possibly picking a less optimal path (or wrong). With KSP, we now
get better nullability information so this new converter takes advantage
of it. To avoid breaking existing clients, this new converter store is
implemented as an alternative implementation that is only enabled in KSP
and can be turned off/on with a flag. That being said, we will still run
java tests with this new converter with hopes to turn down the old one
eventually.

This new converter store tries to preserve nullability. That means, if
we are writing a nullable field into database, it will first try to
convert it into a nullable db column. Detailed design of the heuristic
can be found here: go/room-null-aware-converter

Moreover, it has the ability to wrap user provided type converters such
that if the converter receives a nonnull value, the new Store can
synthesize a converter that receives null and returns null and use that.
In practical terms, this is great for usability as developers don't need
to worry about creating nullable versions of their converters.

Furthermore, it takes these null checks or up casts into account when
calculating path cost. Last but not least, if it cannot find a type
converter path to return a non-null value, it tries to read the same
value as nullable and if it succeds, wraps it in a null checking
converter. Even though this sounds questionable, we don't always have
the right types from db so it is OK to assume this if developer asked
for it (rather than forcing developer to return nullable). More details
on the justification can be found in the design doc.

There is also an inefficiency in the original type converter where,
while doing N -> N path finding, it always goes from start to end. In
practice, this method usually gets called with N -> 1 or 1 -> N and
always searching from start means branching out more than necessary.
This new implementation always goes from the smaller node set to the
larger size, significantly reducing the branching factor. I noticed this
while profiling the old one so the impact is quite sigificant but didn't
change the original one not to introduce risk.

Relnote: "We've added a new TypeConverter analyzer that takes nullability
information in types into account. As this information is only available
in KSP, it is turned on by default only in KSP. If it causes any issues,
you can turn it off by passing
room.useNullAwareTypeAnalysis=false to the annotation
processor. If that happens, please a file bug as this flag will be
removed in the future.
With this new TypeConverter analyzer, it is suggested to only provide
non-null receiving TypeConverters as the new analyzer has the ability
to wrap them with a null check.
Note that this has no impact for users using KAPT or Java as the
annotation processors (unlike KSP), don't have nullability information
in types."

Bug: 193437407
Test: NullabilityAwareTypeConverterStoreTest, NullabilityAwareTypeConversionTest.
Also added a variant to the java test app even though it is not really a goal
here, it would be good to ensure this new converter store works without KSP.

Change-Id: Ia88f916de3c15424ac8cc275d23223c6b5e47a6d