Inline remotedev-serialize, stop using jsan.

[chrisbobbe]

Following discussion here, we've worked out a way to save some computation time when we periodically save data to persistent storage. The investigation was prompted by profiling data in the initial load (which unfortunately got about seven seconds longer on my device after #4230; we reduced most of that in #4344 and #4345).

While there is still a computation-time period of perhaps 500ms after #4230 which wasn't there before (it's the time spent in transform in api.registerForEvents), and this PR wouldn't shorten that at all, it should shorten a longer period that immediately follows that period, during which the gigantic chunk of just-received data is serialized and persisted. (The screenshot at that link shows artificially fast times; the JS is running on my laptop, not my device, since I'm in debug mode to view profiling data in Chrome DevTools.)

[chrisbobbe]

I'll post some before-and-after performance-measurement charts later.

[chrisbobbe]

See some time measurements for this change (they look encouraging!) here.

[gnprice]

Thanks, this looks great! I especially appreciate the small readable commits, and the clear explanations.

The one thing I'd want to be sure to add to this before merging is some tests. I'm particularly interested in the withTypeKey case we added to the remotedev-serialize tests when we added our round-trip strategy:

  withTypeKey: {
    a: 1,
    __serializedType__: {
      b: [2],
      __serializedType__: {c: [3]},
    },
  }

and I'd also like to see a variation on that where __serializedType__ is a key in an Immutable.Map, now that the ordering has changed between our replacer and the toJSON methods. While we're writing tests for this, it's a good time to also add some simple tests that ZulipVersion and URL and all the rest successfully round-trip.

Oh, relatedly: it'd be good to add an assertion that we aren't accidentally dealing with a toJSON method we didn't expect. That is, in the replacer after all the conditions looking for known interesting classes (but before the generic-object check for SERIALIZED_TYPE_FIELD_NAME in value), something like:

  invariant(typeof origValue.toJSON !== 'function', 'unexpected toJSON');

That should be impossible, unless we accidentally put some interesting type into our Redux state that we didn't intend -- so it'd be good to confirm.

Or in fact, perhaps also this check, to be sure we aren't dealing with something that we won't know how to round-trip:

  invariant(origValue.__proto__ === Object.prototype, 'unexpected class');

That should similarly be impossible, and should show up pretty much immediately in dev if we were introducing a bug where it could happen.

---

One other thing I think would be helpful is to move this serialization code out of store.js and into its own file. As you say, store.js has gotten kind of long; and this code is pretty self-contained and doesn't interact with the rest of it. Could call it src/boot/serialize.js -- I don't love the "boot" name, but it's also where we have ZulipAsyncStorage which is the closest other relative, and we can apply any future renaming to most of this directory all together. To make that separation I'd probably reintroduce the tiny serialize and deserialize functions, in order to be the new module's exports.

The ideal version would be for that move to happen at the start of this branch, before the inlining: have the serialize and deserialize values to use for reduxPersistConfig get imported from the new module, and the remotedev-serialize import, the custom replacer and reviver, and the SERIALIZED_TYPE_FIELD_NAME constant would all live in that module. Then all the inlining would happen there, and I think store.js wouldn't even get touched for the rest of the branch.

A bonus is that it'd make it easy to completely disable ESLint temporarily on the file, which would let a few of the bigger inlinings, like the replacer/reviver definitions in "3/x", be more closely verbatim first and have their style assimilated separately.

It might be pretty annoying to revise this branch to go that way around, though -- one thing the Git merge algorithm is not good at handling is when the code that's being changed got moved (except in one case, AFAIK, which is where the whole file got renamed.) If you spot a way to automate that, it'd be nice, but it's not worth doing it by brute force; I'd be happy with just doing the move at the end of the branch.

[gnprice]

Do we really not use this one?

... I guess we don't! We have a number of plain JS Maps returned by various caching selectors -- but we don't seem to have any in the Redux state itself.

We might at some point find we want to change that, if for some reason the performance tradeoffs favor plain JS Map for a particular subtree of the state. But I guess we can always add support back in then if so.

[chrisbobbe]

Thanks for the review!

If you spot a way to automate that, it'd be nice, but it's not worth doing it by brute force; I'd be happy with just doing the move at the end of the branch.

Ah, OK, yeah—thanks. 🙂

Just pushed a new revision.

[gnprice]

Thanks for the revision! This looks great.

Just a few comments below on small pieces.

[gnprice]

I'm not sure if it's NFC; the `expect` call might have been doing
nothing in the absence of a matcher.

I think that's right. Consider expect(false).toBeFalse() -- that has to succeed, so plain expect(false) can't already cause a failure.

(I could imagine Jest doing something to warn about this -- if you get to the end of a test and there was an expect return value that never got a matcher called on it, complain. But I'd expect it to then do that regardless of the value that was passed in.)

[gnprice]

If we were keeping this test, I think I'd want to make a separate test (aka it) for this expect vs. the one above -- the one above checks that the custom replacer/reviver successfully round-trip, and this one checks that the custom replacer actually took effect.

Then this whole test would be conditional, or else would be in a separate loop of its own, and there wouldn't be a conditional expect inside any test.

But we delete this test a few commits later, so keeping its existing style until then makes good sense.

[gnprice]

I'm not sure why we suddenly have to use `\\` instead of `\` to
escape quotes in the JSON.

Yeah, dunno. My guess is that it's related to a difference in Jest version -- the snapshot format can change with Jest versions. (Similarly, that v1 comment added at the top in the next commit is probably due to a newer Jest version.)

To try to understand this, I went and read the Jest docs page on snapshot testing:
https://jestjs.io/docs/en/snapshot-testing
Didn't get anything on this detail, but I did get a better understanding of why they can be useful!

And I think even the authors of the snapshot feature would not recommend them for testing this code. See those Jest docs, and a post they link to:
https://benmccormick.org/2016/09/19/testing-with-jest-snapshots-first-impressions/
I think this is basically right:

The first thing that became clear to me while using snapshot testing is that they’re not for everything. They are optimized for a different case than normal assertion-based tests.

Classic assertion based tests are perfect for testing clearly defined behavior that is expected to remain relatively stable.

Snapshot tests are great for testing less clearly defined behavior that may change often.

The code being tested here is very much in the first category. The motivating example in the category that snapshots are good for is fragments of UI.

(Which I see the appeal of this design for! I'm now tempted to go try making some snapshot tests for a few of our React components, as an experiment.)

Anyway, no need to convert these tests before merge, but I might make a pass over them later.

[chrisbobbe]

Anyway, no need to convert these tests before merge

Yeah, I almost converted the tests to a style we've been using recently, like in fe6f886, but the branch was getting pretty long.

I might make a pass over them later.

Sure, sounds good. Thanks!

(Which I see the appeal of this design for! I'm now tempted to go try making some snapshot tests for a few of our React components, as an experiment.)

Sure! I think react-test-renderer or Enzyme would be good for that; Enzyme (as I remember, and according to some SO posts like this one) has more functionality, and React officially promotes Enzyme.

[gnprice]

This no longer applies (here and for parse), because they're exported for store.js to use.

[gnprice]

Object.getPrototypeOf

TIL! Definitely cleaner than .__proto__.

boringPrototypes.includes

I'm concerned about this (implicit) loop as potentially a performance issue -- remember that we'll be going through this logic for every value inside every data structure we serialize. I'd prefer to keep it down to a very small number of === checks, and Boolean logic.

I think we can do that by only going through this path when typeof origValue === 'object' && origValue !== null. That eliminates all but Object.prototype and Array.prototype as acceptable prototypes here.

Hmm, or another thought that suggests: the whole if/else if chain, or the bulk of it, could perhaps turn into a switch (Object.getPrototypeOf(origValue)).

[chrisbobbe]

I think we can do that by only going through this path when typeof origValue === 'object' && origValue !== null. That eliminates all but Object.prototype and Array.prototype as acceptable prototypes here.

Ah, right—makes sense, thanks.

Hmm, or another thought that suggests: the whole if/else if chain, or the bulk of it, could perhaps turn into a switch (Object.getPrototypeOf(origValue)).

I think this might make sense. It would be slightly more rigid, in that it builds in the assumption that the distinguishing mark we're looking for is the identity of the first item in the prototype chain.

If the AvatarURL code had worked out slightly differently, and the three subclasses (UploadedAvatarURL, FallbackAvatarURL, GravatarURL) just inherited their serialize and deserialize methods from their AvatarURL superclass, without overriding them, I could see it being convenient to do an if (value instanceof AvatarURL) instead of if (value instanceof GravatarURL) and friends; this kind of simplification wouldn't work with the proposed switch/case.

Also, we use an if (Immutable.Map.isMap(origValue))—we'd have to double-check our expectation that if (origValue instanceof Immutable.Map) would be equivalent before refactoring that to the switch/case.

[gnprice]

I think this might make sense. It would be slightly more rigid, in that it builds in the assumption that the distinguishing mark we're looking for is the identity of the first item in the prototype chain.

If the AvatarURL code had worked out slightly differently, and the three subclasses (UploadedAvatarURL, FallbackAvatarURL, GravatarURL) just inherited their serialize and deserialize methods from their AvatarURL superclass, without overriding them, I could see it being convenient to do an if (value instanceof AvatarURL) instead of if (value instanceof GravatarURL) and friends; this kind of simplification wouldn't work with the proposed switch/case.

True!

There'd still be a pretty simple way to handle that, though:

  case GravatarURL:
  case UploadedAvatarURL:
  case FallbackAvatarURL:
    return { data: AvatarURL.serialize(value), [SERIALIZED_TYPE_FIELD_NAME]: `AvatarURL` };

It involves just a bit more explicit detail about the subclasses than value instanceof AvatarURL would. But I think that isn't too bad of a thing.

Also, we use an if (Immutable.Map.isMap(origValue))—we'd have to double-check our expectation that if (origValue instanceof Immutable.Map) would be equivalent before refactoring that to the switch/case.

Yes. From the implementation, I'm pretty sure those two are equivalent. I think the isFoo form comes from wanting some overlapping categories like isKeyed and isIndexed, which the instanceof approach would only be able to model in a language with multiple inheritance. (It seems to originate, with the same implementation strategy used for isMap now, in immutable-js/immutable-js@921b937.)

Alternatively, if there were a few things (like Immutable.Map in a slightly different possible world) which didn't fit the switch (prototype), we could always handle them as plain conditions the same as before -- just in the default case of the switch, or after the switch.

[chrisbobbe]

Cool, makes sense—I just opened #4362 for this.

[gnprice]

It'd be better to have these above the if (typeof value === 'object' && value !== null && SERIALIZED_TYPE_FIELD_NAME in value) case. Logically they're no less applicable if the object happens to have a property with name SERIALIZED_TYPE_FIELD_NAME.

[chrisbobbe]

Thanks for the review! I just pushed a new revision.