Add record field serialization #342

BenSchwab · 2017-03-22T22:48:23Z

While doing work on LRU cache I realized it would be nice to have the record serializer for record size estimation. So took a crack at closing #265.

Our current network parsing set-up follows this pattern BufferedSource -> Map<String, Object> -> T Data. Where BufferedSource represents a json, and the transformation to Map<String, Object>. This transformation is performed by ResponseJsonStreamReader.

When reading from the cache, we want to go from Cache -> Map<String, Object> where this map must be in nearly the same form as the network response. The difference is that this map replaces objects with CacheReferences.

It seemed bad to duplicate all the code in ResponseJsonStreamReader so I opened ResponseJsonStreamReader for extension, and write a very light CacheJsonStreamReader which strongly types CacheReferences instead of parsing them as strings. I don't love relying on inheritance (although it is very lightweight) so @sav007 if you have any other ideas of how to share the code here, please let me know.

Currently, when reading from the SQL store we get a string. This might be bad for the memory concerns that @digitalbuddha had. So I added a fromBufferedSource to the FieldAdapter (which we use under the hood in fromJson.) But we might need to expand the SQLStore to having some sort of streaming byte array for reading records.

BenSchwab · 2017-03-22T22:49:30Z

apollo-runtime/build.gradle

@@ -36,7 +36,7 @@ dependencies {
  compile dep.supportAnnotations
  compile dep.okHttp
  compile dep.moshi
-  provided project(":apollo-api")
+  compile project(":apollo-api")


I had to change this to compile for the test to work (SQLStoreTest now needs access to Optional in :apollo-api @marwanad any reason this needed to be provided?

Changed it back to provided, but added compile to the test variants.

iirc, at that point, the apollo-api dependency wasn't really needed at all and we weren't' decided on how runtime would look like yet, we only had an Apollo class that wraps Retrofit and a Utils class. At that time, we also changed the plugin to add apollo-api deps to the compile dependency set.

We're gonna have to change it to compile anyways (#311). I guess for now we can keep it this way and I'll revisit this when I reopen #311.

… be changed through 'merge'

sav007 · 2017-03-23T17:44:53Z

apollo-runtime/src/main/java/com/apollographql/android/cache/normalized/Record.java

 public final class Record {

  private final String key;
  private final Map<String, Object> fields;
+  private int sizeEstimateBytes = 0;


can we remove memoizedSize and use instead sizeEstimateBytes = -1

And I can't find the place where we set value to this field?

sav007 · 2017-03-23T17:48:12Z

apollo-runtime/src/main/java/com/apollographql/android/cache/normalized/Record.java

@@ -51,4 +83,17 @@ public String key() {
  public Map<String, Object> fields() {
    return fields;
  }
+
+  public int sizeEstimateBytes() {


Who and when this will be called?

This will be used by a bounded memory LRU cache, (and possibly the sql cache -- though SQL might have a better way to track size). In the memory LRU cache, everytime a record is changed (via a merge) we would have to recalculate a size estimate.

I don't know if I love using the toJSON to determine the size, as that isn't very performant, which will be a requirment for the memory cache.

I'm going to remove the size stuff from this diff, as the serialization for Record is valuable in it's own right.

sav007 · 2017-03-23T17:51:21Z

apollo-runtime/src/main/java/com/apollographql/android/cache/normalized/Record.java

+    return calculateSizeEstimate(FieldsAdapter.create());
+  }
+
+  private int calculateSizeEstimate(FieldsAdapter adapter) {


Can we inject this calculation to the place where we create Record, for instance:
in com.apollographql.android.cache.normalized.sql.SqlStore#cursorToRecord

there we have already String jsonOfFields = cursor.getString(2); so can we take size of jsonOfFields as size for record?

BenSchwab · 2017-03-23T18:31:24Z

Decided to remove the record sizing stuff from this PR. I don't think calculating the size of the json string is the "right" way to do this, especially for the primary use case which is the memory lru cache.

sav007 · 2017-03-23T18:41:56Z

...-runtime/src/main/java/com/apollographql/android/cache/normalized/CacheJsonStreamReader.java

+    Object scalar = super.readScalar(streamReader);
+    if (scalar instanceof String) {
+      String scalarString = (String) scalar;
+      if (CacheReference.isSerializedReference((String) scalar)) {


nit: CacheReference.isSerializedReference(scalarString)

sav007 · 2017-03-23T18:43:55Z

apollo-runtime/src/main/java/com/apollographql/android/cache/normalized/CacheReference.java

+    return new CacheReference(matcher.group(1));
+  }
+
+  public static boolean isSerializedReference(String value) {


nit: should it be called canDeserialize ?

digitalbuddha · 2017-03-23T19:43:59Z

...untime/src/androidTest/java/com/apollographql/android/cache/normalized/sql/SqlStoreTest.java

  public static final String IN_MEMORY_DB = null; //null means db is memory only
  private SqlStore sqlStore;

  @Before
  public void setUp() {
    ApolloSqlHelper apolloSqlHelper = ApolloSqlHelper.create(InstrumentationRegistry.getTargetContext(),
        IN_MEMORY_DB);
-    sqlStore = SqlStore.create(apolloSqlHelper, FieldsAdapter.create(new Moshi.Builder().build()));
+    sqlStore = SqlStore.create(apolloSqlHelper, FieldsAdapter.create());


any reason for this change? I'd prefer to be able to use 1 moshi instance for whole app apollo or otherwise

I might be missing something of how Moshi works, but it seems dangerous to register our BigDecimal converter on a global Moshi instance, in case there is an existing converter. (Currently, it seems we do not allow a user to pass in a Moshi instance to Apollo anyway).

We could register these convertors on the Apollo's Moshi instance in ApolloClient. But that Moshi instance seems primarily concerned with serializing variable objects, and initializing cache serializers there would take some thought of how to do it properly without spaghetti dependencies in the ApolloClient builder.

My thinking was from a performance standup. Moshi caches adapters but it's on a per instance level. So say I have same date converter on app and apollo client, I'd prefer to not have to cache both. Currently our app works with 1 instance of gson. It doesn't have to be solved now. I can do a pr if it becomes an issue

digitalbuddha · 2017-03-23T19:49:54Z

apollo-runtime/src/main/java/com/apollographql/android/cache/normalized/FieldsAdapter.java

+
+  public Map<String, Object> fromJson(String fields) throws IOException {
+    final BufferedSource bufferSource = Okio.buffer(Okio.source(new ByteArrayInputStream(fields.getBytes())));
+    return fromBufferedSource(bufferSource);


nitpick, it will read better as from(bufferedSource)

digitalbuddha · 2017-03-23T19:55:59Z

apollo-runtime/src/main/java/com/apollographql/android/cache/normalized/FieldsAdapter.java

+
+  public Map<String, Object> fromBufferedSource(BufferedSource bufferedSource) throws IOException {
+    CacheJsonStreamReader jsonStreamReader
+        = new CacheJsonStreamReader(new BufferedSourceJsonReader(bufferedSource));


Personally I like APIs with static calls for object creation like
ApolloReader.cacheJsonStream(ApolloReader. bufferedSourceJson(bufferedSource)
This way you can do static imports and become cacheJsonStream(bufferedSourceToJson(source))

digitalbuddha

Nothing major just few nitpicks :-)

Add record field serialiation, record size estimation

7e37048

BenSchwab commented Mar 22, 2017

View reviewed changes

BenSchwab added 3 commits March 22, 2017 16:12

Change 'setField' in Record to builder pattern, as Record should only…

3eeb615

… be changed through 'merge'

write better javadoc

129e41f

keep provided add api to test config

e2ea512

BenSchwab requested review from brianPlummer, sav007, digitalbuddha and marwanad March 22, 2017 23:51

fix checkystyle

a3debf9

BenSchwab added Status: Review Needed 🗄️ Normalized cache 🚗 Runtime labels Mar 23, 2017

sav007 reviewed Mar 23, 2017

View reviewed changes

remove record sizing

20425b8

BenSchwab changed the title ~~Add record field serialiation, record size estimation~~ Add record field serialization Mar 23, 2017

sav007 reviewed Mar 23, 2017

View reviewed changes

sav007 approved these changes Mar 23, 2017

View reviewed changes

fix nits

11ee673

digitalbuddha reviewed Mar 23, 2017

View reviewed changes

digitalbuddha approved these changes Mar 23, 2017

View reviewed changes

BenSchwab added 2 commits March 23, 2017 14:04

more semantic reader names

6f150e8

fix checkstyle

6fc6ebc

BenSchwab merged commit 057cfc3 into master Mar 23, 2017

BenSchwab deleted the bschwab--record-field-serialization branch March 23, 2017 23:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add record field serialization #342

Add record field serialization #342

BenSchwab commented Mar 22, 2017 •

edited

Loading

BenSchwab Mar 22, 2017

BenSchwab Mar 22, 2017

marwanad Mar 23, 2017 •

edited

Loading

sav007 Mar 23, 2017

sav007 Mar 23, 2017

sav007 Mar 23, 2017

BenSchwab Mar 23, 2017 •

edited

Loading

sav007 Mar 23, 2017

BenSchwab commented Mar 23, 2017

sav007 Mar 23, 2017

sav007 Mar 23, 2017

digitalbuddha Mar 23, 2017

BenSchwab Mar 23, 2017

digitalbuddha Mar 23, 2017

digitalbuddha Mar 23, 2017

digitalbuddha Mar 23, 2017 •

edited

Loading

digitalbuddha left a comment

Add record field serialization #342

Add record field serialization #342

Conversation

BenSchwab commented Mar 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marwanad Mar 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenSchwab Mar 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenSchwab commented Mar 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

digitalbuddha Mar 23, 2017 • edited Loading

Choose a reason for hiding this comment

digitalbuddha left a comment

Choose a reason for hiding this comment

BenSchwab commented Mar 22, 2017 •

edited

Loading

marwanad Mar 23, 2017 •

edited

Loading

BenSchwab Mar 23, 2017 •

edited

Loading

digitalbuddha Mar 23, 2017 •

edited

Loading