-
Notifications
You must be signed in to change notification settings - Fork 52
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Reuse environment definitions in ToJSON
and FromJSON
#2107
Comments
This may actually be a good application for the (heretofore mythical) |
I think even Outputting JSON is not done on main thread, so it would not freeze the game. |
Good point. Yes, we should not succumb to premature optimization here. Let's start with the simplest thing that works and we can optimize it later if necessary. |
After some discussion with @xsebek on Discord, here's a sketch of an idea. Currently data CtxStruct t = CtxEmpty | CtxSingle Var | CtxDelete Var (Ctx t) | CtxUnion (Ctx t) (Ctx t)
data Ctx t = Ctx { ctxMap :: Map Var t, ctxName :: CtxName, ctxStruct :: CtxStruct t } The To generate unique I had been thinking in terms of storing each tree node indexed by name in a map, or something horrendous like that. It was @xsebek's idea that all we need to do is store some unique names alongside just so we can use them to compare for equality. When serializing, we can keep track of a set of |
@byorgey this sounds great! 👍 Will this also work for type context, or will it use the old version? 🤔 |
Type contexts also use |
Working on a proof of concept, watch for a PR soon... 😃 |
Is your feature request related to a problem? Please describe.
TLDR: the problem is that
_envVars
inside_envVars
cause exponential JSON size.Take this simple example:
And observe the growth of JSON output:
This makes it impossible to get to other useful parts of robot JSON, like the log, if the program has enough definitions:
Describe the solution you'd like
The definitions should be reused - the inner references should link (
{"link": "m8"}
) to outer definition.If we can check that the definitions are the same, we could prune them from inner scope:
Describe alternatives you've considered
The current derived instance is broken, so maybe we could only keep the top environment, or none at all.
The text was updated successfully, but these errors were encountered: