Save and read from new serialization where possible #3566

pbiggar · 2022-03-11T17:18:43Z

Does most of #3540.

Since serialization with the legacy server is so slow, both BwdServer and ApiServer give bad experiences. Using the editor is a very bad experience for even medium-sized canvases, as the API call to initial_load makes one call to the legacy_server for each toplevel (including deleted toplevels). Calling the BwdServer is also bad, as it needs to load every function and database regardless of whether the handler uses them.

The slow part is not actually calling the server or deserializing the binary, but rather decoding the JSON that the server returns. Encoding the JSON isn't super great either but decoding it is extremely bad.

Instead, this PR lets us save handlers using MessagePack.CSharp binary serialization format. This is not as fast as the OCaml binary serializer, but it's much much much much better than using the legacy_server.

This works by adding new columns on the toplevel_oplists table (which is where the data is currently stored). In addition to the two rows where the ocaml-serialized data is stored (data and rendered_oplist_cache), we add two more for the fsharp-serialized data (oplist and oplist_cache).

When saving data in F#, we save all 4 fields. When saving data in OCaml, we save the 2 ocaml-serialized fields, and set the other two to NULL.

When reading data in F#, we read all 4 fields and pick appropriately: if the new ones are NULL we'll deserialize via the legacy server). When reading in OCaml, we only use the two old columns.

In this way, F# writes are still readable from OCaml. This is important because the bwdservers and queueservers are still ocaml.

OCaml writes are also still readable from F#, because they'll delete (overwrite with null) old fsharp-serialized values, and F# will read the new values (deserializing using the legacy server).

I also want to automatically upgrade values on read. However, I'm going to leave that to a future PR as that is a little more sketch and I'd like to get this merged without increasing scope.

fsharp-backend/src/LibBackend/BinarySerialization.fs

StachuDotNet · 2022-03-11T18:37:26Z

Maybe it'll be clearer to me after more reading, but does this prepare us for a case where the shape of Spec or Op changes? Or will the binary serialization fail then? (one nice thing about serializing and then deserializing JSON is that as long as your changes are additive, it works out okay.

or maybe we don't anticipate those types to change? if we're relying on that somehow, let's add a note at least.

fsharp-backend/src/Prelude/Prelude.fs

fsharp-backend/src/LibBackend/Serialize.fs

pbiggar · 2022-03-11T19:13:58Z

Maybe it'll be clearer to me after more reading, but does this prepare us for a case where the shape of Spec or Op changes? Or will the binary serialization fail then? (one nice thing about serializing and then deserializing JSON is that as long as your changes are additive, it works out okay.

or maybe we don't anticipate those types to change? if we're relying on that somehow, let's add a note at least.

Yeah, I should add some notes and some tests around this.

Currently I have no idea what happens if we change the Op or Toplevel or Expr types. In the OCaml serializers, we had to be very careful, and were basically only able to add extra variants to existing sumtypes. And so we had a few checks:

a file was generated that represented the serialization format. If a new serialization format was created, we would know we had changed the format
a digest representing the format was stored in the DB (toplevel_oplists.digest)
we had a big set of warnings on the affected files and types

I think I can get a similar level of confidence with the following:

generate a file, which we commit, representing a serialized value (shows if things have changed)
a test that we can read that file and get the right answer
a test that we can serialize it and get the same value
warnings around ProgramTypes.fs
refactor ProgramTypes.fs to just represent the program types
some quick manual tests as to what happens if we make some changes to the files

StachuDotNet

Looks good, apart from the comment I made earlier:
Let's say we want to add some property to ProgramTypes.Op.CreateDB in a few months. I think right now with JSON, we wouldn't have any issue deserializing old data into the new type. Does that hold true with MessagePack? I did some quick searches of binary serializers and msgpack specifically, (just a minute) and didn't see an obvious conclusion.

Edit, reading, just saw your response

StachuDotNet · 2022-03-11T19:27:07Z

That sounds like a good plan

pbiggar · 2022-03-11T19:49:38Z

Looks good, apart from the comment I made earlier: Let's say we want to add some property to ProgramTypes.Op.CreateDB in a few months. I think right now with JSON, we wouldn't have any issue deserializing old data into the new type. Does that hold true with MessagePack? I did some quick searches of binary serializers and msgpack specifically, (just a minute) and didn't see an obvious conclusion.

Right now we don't use JSON for serialization - we use the bin_io serializers. We also can't use JSON as it's much too slow.

The existing bin_io serializers that we use also have this problem. The solution we use is to never add properties to Op.CreateDB, instead we create Op.CreateDBButWithAnotherProperty (good example actually since we already have Op.CreateDBWithBlankOr, which was a sitation exactly like this).

I don't know if that holds true for MessagePack, but I'd be very surprised if it doesn't. Certainly I will be testing.

This makes me realize I should also add exception handling during the new binary serialization, and add a fallback to use the old serializers.

The current version of the C# serializer supports a string key serializer, which is a bit more resilient to versioning changes. It's the default right now, but I think that we would be better with very explicit changes and tests rather than relying on this behaviour. It's also 2x slower with this and generates data 2x bigger, so I'd prefer to change away from this now.

pbiggar · 2022-03-11T20:02:52Z

pbiggar · 2022-03-14T17:58:52Z

Remove DB migration variants

pbiggar · 2022-03-15T04:48:09Z

Changes since last review (starts at 6bebf89):

I change the format to use ints instead of strings for record member names. This dramatically dropped the size of the generated data, to roughly match the old sizes (vs approximately 2x before).
enable compression for oplist serialization. Since there's a lot of duplicate here (eg, if you press two keys you'll get two nearly identical SetHandler ops), this reduces stored size by about 15x.
made the options part of the serialization function, not something global that needs to be initialized.
add a big test case hitting every form of every type in ProgramTypes
made a set of test cases to ensure the format doesn't change. I won't describe it here as it should be described enough in the comments (and if not I need to imprve the comments).
add a lot of discussion of what changes can safely be made
remove old DB migration Ops since they're not used anywhere

pbiggar · 2022-03-15T04:48:57Z

@StachuDotNet Ready for another review.

fsharp-backend/tests/Tests/SqlCompiler.Tests.fs

fsharp-backend/src/LibBackend/BinarySerialization.fs

StachuDotNet · 2022-03-15T13:19:01Z

fsharp-backend/src/LibBackend/BinarySerialization.fs

+let serializeOplistToJson (oplist : PT.Oplist) : string =
+  MessagePack.MessagePackSerializer.SerializeToJson(oplist, optionsWithZip)


should we mark this fn as test-only in some way? i.e. Do we anticipate using this outside of tests at all in the future?

I'll make a "Test" module within the file for this (and we can use that pattern elsewhere).

fsharp-backend/src/LibExecution/ProgramTypes.fs

StachuDotNet · 2022-03-15T13:25:36Z

fsharp-backend/tests/Tests/BinarySerialization.Tests.fs

+/// The test values below are used to check the exact output of test file. So we need
+/// the test inputs to be consistent, which is why we never use `gid ()` below, or
+/// FSharpToExpr functions.
+let testExpr =


How'd you get to this expression - extract it from a live canvas?

I aimed to write one that hit every type of the variant.

fsharp-backend/tests/Tests/BinarySerialization.Tests.fs

StachuDotNet · 2022-03-15T13:31:15Z

fsharp-backend/tests/Tests/Canvas.Tests.fs

+           [ hop handler ],
+           PT.Toplevel.TLHandler handler,
+           Canvas.NotDeleted)
+          (f.tlid, [ PT.SetFunction f ], PT.Toplevel.TLFunction f, Canvas.NotDeleted) ]


I find myself asking "... what's 'f' here?". Really easy to lose context in a PR review. Could we rename?

I don't disagree, but I just hit this from a module rename and didn't touch it otherwise. Would rather not increase scope on this.

fsharp-backend/src/LibBackend/PackageManager.fs

fsharp-backend/src/LibExecution/ProgramTypesParser.fs

StachuDotNet

This is looking really great! Code is reading much nicer now, generally.
I like the way stability of the binary serialization is ensured. I wonder if there's a nice way to ensure that the test in BinarySerialization.test.fs contains all available Ops (i.e. walk the tree and error if any ops aren't included) or if that's worth exploring.

enable compression for oplist serialization. Since there's a lot of duplicate here (eg, if you press two keys you'll get two nearly identical SetHandler ops), this reduces stored size by about 15x.

awesome.

Just a few pending comments/questions, but this looks good to me!

pbiggar · 2022-03-15T15:13:27Z

Going to do a roundtrip test of all existing canvases to be sure I didn't fuck this up, then merge.

These are not used by anything, and are not created by the frontend in practice. There are definitely some of these values in the DB -- during the migration they'll be removed, and will not throw errors.

Co-authored-by: Stachu Korick <hello@stachu.net>

StachuDotNet · 2022-03-15T16:39:35Z

fsharp-backend/src/LibBackend/BinarySerialization.fs

+module Test =
+  let serializeOplistToJson (oplist : PT.Oplist) : string =
+    MessagePack.MessagePackSerializer.SerializeToJson(oplist, optionsWithZip)


I really like this pattern!

The migration will be run by the ocaml machines on startup, so it has to be idempotent. As it was, it would fail if the columns had already been added (and of course we are adding the columns by hand before deployment, so they will be there). It is now idempotent. Tested by hand by running the migration, seeing it fail, then fixing it and running it again and seeing it go through without incident.

pbiggar requested a review from StachuDotNet March 11, 2022 17:48

StachuDotNet reviewed Mar 11, 2022

View reviewed changes

fsharp-backend/src/LibBackend/BinarySerialization.fs Outdated Show resolved Hide resolved

StachuDotNet reviewed Mar 11, 2022

View reviewed changes

fsharp-backend/src/Prelude/Prelude.fs Show resolved Hide resolved

StachuDotNet reviewed Mar 11, 2022

View reviewed changes

fsharp-backend/src/LibBackend/Serialize.fs Show resolved Hide resolved

StachuDotNet reviewed Mar 11, 2022

View reviewed changes

fsharp-backend/src/LibBackend/Serialize.fs Outdated Show resolved Hide resolved

StachuDotNet approved these changes Mar 11, 2022

View reviewed changes

pbiggar force-pushed the paul/serialization-4 branch from 27a6431 to efb2c24 Compare March 15, 2022 04:15

StachuDotNet reviewed Mar 15, 2022

View reviewed changes

fsharp-backend/tests/Tests/SqlCompiler.Tests.fs Show resolved Hide resolved

StachuDotNet reviewed Mar 15, 2022

View reviewed changes

fsharp-backend/src/LibBackend/BinarySerialization.fs Outdated Show resolved Hide resolved

StachuDotNet reviewed Mar 15, 2022

View reviewed changes

fsharp-backend/src/LibExecution/ProgramTypes.fs Show resolved Hide resolved

StachuDotNet reviewed Mar 15, 2022

View reviewed changes

fsharp-backend/tests/Tests/BinarySerialization.Tests.fs Show resolved Hide resolved

StachuDotNet reviewed Mar 15, 2022

View reviewed changes

fsharp-backend/src/LibBackend/PackageManager.fs Show resolved Hide resolved

StachuDotNet reviewed Mar 15, 2022

View reviewed changes

fsharp-backend/src/LibExecution/ProgramTypesParser.fs Outdated Show resolved Hide resolved

StachuDotNet reviewed Mar 15, 2022

View reviewed changes

fsharp-backend/src/LibExecution/ProgramTypesParser.fs Outdated Show resolved Hide resolved

StachuDotNet requested changes Mar 15, 2022

View reviewed changes

pbiggar added 3 commits March 15, 2022 15:40

Add CLEANUPs

90fb57d

Add mapWithConcurrency

6c333b6

Add ability to serialize with MessagePack, and tests

428bd0b

pbiggar and others added 23 commits March 15, 2022 15:46

Add binary serialization file tests

6c34177

Add way to regenerate test files

4432f0a

Add oplist format files to the repo

526c127

Make the serialization format stable

704270f

Update comment

f7ee3a6

Add more docs

d751d11

Comment out debug

ca7cb3f

Remove string prettification. It doesn't help

14cfa61

More serialization documentation

3f655c5

Add reading the binary serialization to the test

b404968

Add more comments about what's allowed and what isn't

b045004

Complete notes

20edfa5

Improve notes

1a12f3a

We don't serialize secrets using BinarySerialization

7443202

Move secrets down to be with the other non-serialized values

5c0e870

Remove unused ops

a0d15cd

These are not used by anything, and are not created by the frontend in practice. There are definitely some of these values in the DB -- during the migration they'll be removed, and will not throw errors.

Update fsharp-backend/src/LibBackend/BinarySerialization.fs

676df9b

Co-authored-by: Stachu Korick <hello@stachu.net>

Update fsharp-backend/src/LibExecution/ProgramTypesParser.fs

cc4ff53

Co-authored-by: Stachu Korick <hello@stachu.net>

Update fsharp-backend/src/LibExecution/ProgramTypesParser.fs

797b008

Co-authored-by: Stachu Korick <hello@stachu.net>

Move to a Test module

b640f8a

Fix copy error

c5d2d43

Fix erroreous call to ToString

42367b2

Fix rebase error

6d46a1f

pbiggar force-pushed the paul/serialization-4 branch from 1c05d1a to 6d46a1f Compare March 15, 2022 15:48

StachuDotNet reviewed Mar 15, 2022

View reviewed changes

pbiggar added 2 commits March 15, 2022 17:33

Ensure we capture diagnostic info if a serialization fails

fab7215

pbiggar mentioned this pull request Mar 15, 2022

Use Task.mapWithConcurrency instead of chunking crons #3575

Merged

pbiggar merged commit e40eed4 into main Mar 16, 2022

pbiggar deleted the paul/serialization-4 branch March 16, 2022 14:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Save and read from new serialization where possible #3566

Save and read from new serialization where possible #3566

pbiggar commented Mar 11, 2022 •

edited

Loading

StachuDotNet commented Mar 11, 2022

pbiggar commented Mar 11, 2022

StachuDotNet left a comment •

edited

Loading

StachuDotNet commented Mar 11, 2022

pbiggar commented Mar 11, 2022

pbiggar commented Mar 11, 2022 •

edited

Loading

pbiggar commented Mar 14, 2022 •

edited

Loading

pbiggar commented Mar 15, 2022

pbiggar commented Mar 15, 2022

StachuDotNet Mar 15, 2022 •

edited

Loading

pbiggar Mar 15, 2022

StachuDotNet Mar 15, 2022

pbiggar Mar 15, 2022

StachuDotNet Mar 15, 2022

pbiggar Mar 15, 2022

StachuDotNet left a comment

pbiggar commented Mar 15, 2022

StachuDotNet Mar 15, 2022

		let serializeOplistToJson (oplist : PT.Oplist) : string =
		MessagePack.MessagePackSerializer.SerializeToJson(oplist, optionsWithZip)

Save and read from new serialization where possible #3566

Save and read from new serialization where possible #3566

Conversation

pbiggar commented Mar 11, 2022 • edited Loading

StachuDotNet commented Mar 11, 2022

pbiggar commented Mar 11, 2022

StachuDotNet left a comment • edited Loading

Choose a reason for hiding this comment

StachuDotNet commented Mar 11, 2022

pbiggar commented Mar 11, 2022

pbiggar commented Mar 11, 2022 • edited Loading

pbiggar commented Mar 14, 2022 • edited Loading

pbiggar commented Mar 15, 2022

pbiggar commented Mar 15, 2022

StachuDotNet Mar 15, 2022 • edited Loading

Choose a reason for hiding this comment

pbiggar Mar 15, 2022

Choose a reason for hiding this comment

StachuDotNet Mar 15, 2022

Choose a reason for hiding this comment

pbiggar Mar 15, 2022

Choose a reason for hiding this comment

StachuDotNet Mar 15, 2022

Choose a reason for hiding this comment

pbiggar Mar 15, 2022

Choose a reason for hiding this comment

StachuDotNet left a comment

Choose a reason for hiding this comment

pbiggar commented Mar 15, 2022

StachuDotNet Mar 15, 2022

Choose a reason for hiding this comment

pbiggar commented Mar 11, 2022 •

edited

Loading

StachuDotNet left a comment •

edited

Loading

pbiggar commented Mar 11, 2022 •

edited

Loading

pbiggar commented Mar 14, 2022 •

edited

Loading

StachuDotNet Mar 15, 2022 •

edited

Loading