search

dkrasner / Simpletalk

Apache License 2.0
53 stars 3 forks source link

Default Properties Serialization #110

Closed darth-cheney closed 3 years ago

darth-cheney commented 3 years ago

What

This PR is a direct fix for Issue #78. In order to reduce the size of our serializations (and snapshots), we no longer serialize properties whose values are equivalent to the default property value.

Implementation

The implementation of this capability is simple and occurs in two places.

First, we attach a prop.default property in the BasicProperty constructor that caches the provided default value (see here).

Doing so allows us to "filter out" any part properties that are currently equal to their default value when serializing. This occurs in the serialize() method of Part (and also the overrides in Image and WorldStack).

Comparison

Here is an overview of the reduced size of the Home snapshot, which is notable for the large number of Parts it contains: Snapshot size
home.html 6.2mb
home-compressed.html 2.5mb

That is a 60% decrease in size.

Possible Steps Pre-Merging

Should we take this opportunity to re-serialize and save all snapshots in the examples folder, then commit to this branch prior to merging?

dkrasner commented 3 years ago

@darth-cheney Ok this looks good and is simple.

slight correction, the reduction on home.html is

Snapshot size
home.html 3.7mb
home-compressed.html 2.5mb

The drop 6.2MB -> 3.7MB comes from the earlier image serialization PR, which this is rebased on top. But of course that's still a 30% reduction for very little change!

One of the things I was was thinking about is whether we could get better compression if we allowed for properties set in Part.init() to be default, since they are really default for the corresponding part. I did so in this commit. Unfortunately it wasn't as much as I expected, taking both the original home.html and home_compressed.html to 2.2Mb, but it could make a bigger difference in a stack where the defaults are respected .... If you don't think it's worth it just reset the head to previous.

[Note: at first I was thinking of making setFault=true but there are too many places where you don't want this and it was causing crashes. But this could be the right thing to do, would just require a close combing of the code.]

There is a de-serialization test that is failing, prior to my commit.

Otherwise this is definitely a win.

darth-cheney commented 3 years ago

One of the things I was was thinking about is whether we could get better compression if we allowed for properties set in Part.init() to be default, since they are really default for the corresponding part. I did so in this commit.

Good catch. I didn't even think of those.

There is a de-serialization test that is failing, prior to my commit.

Will fix and then merge in