Skip to content
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

VScript serialisation rewrite #221

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from

Conversation

samisalreadytaken
Copy link

@samisalreadytaken samisalreadytaken commented Dec 14, 2022

Current implementation had too many problems and unfixable issues with the approach it took. I rewrote SquirrelVM::WriteState() and SquirrelVM::ReadState() from scratch, fixing not only the linked issues but also various other undocumented issues.

This PR fixes serialisation of attributes, threads and generators, improves performance by skipping the VM stack alltogether in serialisation - previously it would push and pop every object to the VM stack, unnecesarily adding a lot of overhead. A very rudimentary test showed this code is approximately 12% faster in WriteState, 30% faster in ReadState.

This obviously is not backwards compatible. The only way to keep it compatible would be keeping a copy of the old read code for matching version numbers.


Does this PR close any issues?

fixes #99 (although this was already marked fixed, I believe this is the same issue as #219)

PR Checklist

  • My PR follows all guidelines in the CONTRIBUTING.md file
  • My PR targets a develop branch OR targets another branch with a specific goal in mind

@samisalreadytaken samisalreadytaken changed the base branch from master to develop December 14, 2022 23:37
@@ -22,13 +22,15 @@
#include "sqstdmath.h"
#include "sqstdstring.h"

// HACK: Include internal parts of squirrel for serialization
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still a HACK, in fact it's even a bigger reach into the internals now.

It would be good to look at what can be done to avoid over reaching too far into internals and abstract away what's needed, even if the parts that depend on internals are in their own cpp file.

It's worth also looking to push it up stream to the squirrel lang repo what ever is needed to have proper serialization, this initial version was very hacky and needed to reach into the internals, I did have plans to push more upstream.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implementing complete serialisation natively in Squirrel would be very a large change (and an undertaking), I don't see such feature being accepted any time soon if at all.

Having a substantial serialisation does require accessing the internals anyway. Adding interface to access what is required is also unrealistic since what is required is everything.

@samisalreadytaken samisalreadytaken force-pushed the vscript-saverestore branch 2 times, most recently from cdc8741 to 5eccbe3 Compare December 26, 2022 10:35
@samisalreadytaken samisalreadytaken marked this pull request as ready for review December 26, 2022 10:35
Copy link
Member

@Blixibon Blixibon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've noticed this PR is no longer in a draft state, so I'm assuming you're done with these changes and you believe this is safe to be merged. I'm not familiar enough with most of the code involved to give a fair assessment on it, but I conducted a light review and I didn't see anything that stood out to me.

Due to the sensitivity of these changes, I'm going to wait before merging this to ensure there's no remaining objections or comments surrounding it.

@samisalreadytaken
Copy link
Author

samisalreadytaken commented Oct 25, 2023

I think it's ready. I don't remember having any ref discrepancies left on save/restore.

Previous save-breaking changes doesn't seem to have increased saverestore version numbers, perhaps version increment is not really needed.

@samisalreadytaken samisalreadytaken force-pushed the vscript-saverestore branch 5 times, most recently from 5fba33e to 1abf0ce Compare January 13, 2024 13:07
@samisalreadytaken samisalreadytaken force-pushed the vscript-saverestore branch 2 times, most recently from d1f80ea to 8c0b3f2 Compare January 19, 2024 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants