-
Notifications
You must be signed in to change notification settings - Fork 162
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
AGS 4: properly support managed pointers in managed structs #1923
AGS 4: properly support managed pointers in managed structs #1923
Conversation
698287b
to
8994b4e
Compare
So... in overall this is ready for testing. The only mandatory thing missing is compiler tests update, where the bytecode tests are failing because of the new opcodes (and different number of args). I will look into this soon. But while writing this, I suddenly realized there's a serious loophole with game saves. I will explain this later, in the second part of the comment. For now, the manual tests might include following:
NOTE The game save issue While I've been writing this, it suddenly came to me that the (un)serialized typeids may get broken if RTTI changes. And more...
|
Eh, thinking more about game saves problem; there's another major issue: the types declared in room scripts. Because no room scripts are loaded at the time when the save is restored (and only 1 particular room will be loaded later), we won't be able to "match" the objects created from room types. So, the only solution I see at the moment is to have either full RTTI written in saves, or at least a limited pieces of data necessary for supporting the features (like nested pointers in our case). Then, upon restoring a save, create "temporary types", and match & remap these as more scripts are loaded. I'll have to think more about this. |
ee629c3
to
9d764f6
Compare
03c1ab0
to
5260515
Compare
4a4b240
to
9a17bd4
Compare
Hmm, well, with the latest changes the "saves problem" seems to be formally resolved in a minimal necessary way; but the code is quite ugly, so I'll take couple more days to tidy it, and then write a complete explanation of what it does. Note: by "minimal necessary" solution I mean handling following cases:
|
9a17bd4
to
c72d95d
Compare
Alright, it took me unexpected extra time to figure out something about restoring saves (because I did not realize this problem early enough), but I suppose that current implementation has a "minimal necessary" solution for it. By that I mean that it will formally work, except for a few niche cases. It will definitely work for a unchanged (finalized) game script. There are options for enhancing this in the future, both resolving some of the niche cases, and improving performance. But, general words aside, following is a quick recap of problems with saves, and how the engine will deal with that now.
This is how I tried to solve this.
This way
|
f895a59
to
266a3b6
Compare
Well, I think this is working, overall. I made certain that the new opcodes and RTTI generation itself could be disabled completely from compilation, in which case compilers will write old opcodes and also error on nested managed pointers (see compiler options SCOPT_RTTI and SCOPT_RTTIOPS. So the whole feature may be turned off if something bad happens. It is also possible to write a fallback in script, using SCRIPT_EXT_NESTEDPOINTERS macro. (Although that's rather a formality, as it may be difficult to write two full variants of your script with and without this feature.) The object disposal is fine, at least according to my tests. My only doubts at the moment are related to saves problem, as mentioned above, but this is maybe because the whole situation about AGS saves relying on exact game data state is ugly. The implemented solutions results in save files being several tens of Kilobytes larger, and having extra processing after the load. It might be possible to optimize probably, and also possible to replace with some other solution at the cost of changing the save format in particular component. I think the biggest TODO after this will be to implement some kind of a managed pool statistics, which are gathered and optionally printed somewhere. Testing proper disposal manually may be a tedious task, and only suitable if you have few big objects in memory - then you can clearly notice the memory change using trivial system tools. But this kind of test may not be suitable for a regular game run. In regards to this PR, the only thing remaining is fixing compiler tests. |
19ebcc5
to
d7dd2e2
Compare
* Move DynArray unserialization to the AGSDeSerializer class, because ManagedObjectPool's purpose is a storage, not object creation operations. * Unified DynArray and UserObject deserialization, including use of Stream instead of copying values from the raw buffer. * Add some comments for the future refactor considerations.
There are 2 purposes here: 1. Be able to remap restored managed objects that use typeids (dyn arrays and managed user structs) after restoring a save in an updated game; as the game may have slightly (or largerly) different table of types with entries added and reordered. 2. Be able to generate placeholder RTTI entries for types declared in rooms. As only 1 room (and room script) is loaded by the engine at a time, the global RTTI table may not have information about all the types when restoring a save. The placeholder types have just enough information to allow managed object identification and disposal. What is done: 1. Added ICCTypeidBased interface, which simply adds a RemapTypeid method. CCDynamicArray and ScriptUserObject inherit this interface, as they are the only objects that rely on typeids now. 2. Global AGSDeSerializer is removed, and instead one added in RestoredData struct, as it is really only needed during save restore. So now it will be created for the restore process and disposed right after. 3. AGSDeSerializer records the list of dynamic arrays and user objects handles while restoring them. 4. AGSDeSerializer provides a method for remapping typeids in all the objects recorded in aforementioned way. 5. The DoAfterRestore function now does two things: a) if a RTTI was loaded from save, joins one to the global collection, and receives typeid map for remapping, b) asks AGSDeSerializer to remap recorded list of managed objects. 6. Finally, added RTTI save component, for writing and reading minimal necessary part of RTTI. NOTES: * Some parts of this process are still relatively ugly. There's a question of whether tasks are delegated reasonably among various engine components. * The typeid remapping may and should be optimized, but I left this out of this change, and wrote some comments for the future.
The implementation is based on the concepts described in Python dev docs: https://devguide.python.org/internals/garbage-collector/ It's not aiming to be as efficient at the moment, because the way AGS managed objects are stored are preventing this. But future optimization is always possible.
The purpose is to hide the actual plugin's manager behind a proxy, as plugin's interface may not fully comply to our internal one. This prevents errors if one of the extended methods is called by the engine. The base ICCDynamicObject interface currently consists of only few methods, which are used only once in the object's lifetime, and therefore this wrapper should have a small overhead.
cf5cc93
to
989f462
Compare
After the #1990 the script performance got improved significantly, and it's time to compare once again: Testing the 3D space sim prototype So, it looks like with this feature there's a stable drop by 2-3 fps, but compared to the new performance level it's mere %. As i mentioned above, there are couple of places which may be optimized probably in the future. Also tested racing game, it's a little difficult because the fps jump all the time, but the "stable" highest values are: |
From originally 65 fps up to 110 fps: that's great improvement! So the combination seems to do it best – a more efficient bytecode interpreter and more efficient bytecode. |
I'm not quite up-to-date yet: Do we do both reference counting and trace-and-sweep garbage collection to reclaim unused memory? If so, can we perhaps do away with the reference counting and only rely on the collector? Because I think I read somewhere some time ago that reference counting can potentially be slower than garbage collecting, amortized over bytecode instructions executed. I don't know whether that could be relevant in our system. |
The GC I wrote uses reference counter to find out which objects are still attached to pointers in script (or from internal storage in the engine). It uses the idea described here: https://devguide.python.org/internals/garbage-collector/ In order to do this without ref counter, we'd have to keep records of all the pointer variables in global script memory at least, and then trace managed objects starting from those variables that have a valid handle. From what I understood this uses concept of a "gcroot" object. For that to work we'd need to generate a list of pointers in script data, similarly to how the RTTI is generated now. This was my first thought, but I decided to use gc refcount method described in the linked article, as it did not require any additional script data. I guess later someone could experiment with the "gcroot" option and see if it works faster in AGS games. One thing to consider, because the nested or cross-referenced managed structs were not a thing before, right now there are ZERO released games where detached objects can be a thing, so there's practically no real test case ready. |
@fernewelten oh, by the way, there are not explicit bytecode instructions that do ref count change. Ref count is changed on pointer assignment, that would be MEMWRITEPTR, MEMINITPTR and MEMZEROPTR. So, there's no bytecode to save, only internal calls to the managed pool. |
Well, this is generally working, the only issue I see here is testing. Manual testing will only show that the pointers themselves work. What could be a convenient way to add some automated aid here? For example, I could add a compilation flag for logging managed pool stats, and then print number of allocated objects, once per N object allocations. |
It all depends on the problems you want to detect, so what kind of problems the GC can have?
In general my head can't think many possible things that could go wrong there - beyond a case where the behavior itself is wrong, but the n I don't know how logging something could help. There could also be something similar to Ctrl+G to print some stats in a Display (maybe add to the Debug(a,b) things). |
Primarily the correctness of GC and memory leaks, to know that this feature works formally at least. |
In theory, if we could run it on a script and get some picture of it's state, say before and after some script point, we could write a test to verify it's not incorrect. If there was some way to dump the GC state to an String at runtime these tests could be written in AGS Script itself with regular Editor and Engine. Problem is, I think the different states even on an empty game would have a lot of elements you don't care I think, so one may need to figure something to parse out the parts of the state that don't matter. Alternatively these could only spit these states to a text file. Of course this testing, for now, is something we would make and run at one time, and not something to continuously verify games in general, I don't have many ideas on that. |
13c6cca
to
42726df
Compare
42726df
to
4569865
Compare
Added some trivial collected stats to the managed pool, printed every now and then after N GC runs, on room change and game exit. |
WARNING: this requires #1922 to work.
(pr's branch is based on top of that pr for easier testing)
This resolves a long existing problem of AGS script not supporting managed pointers inside user's managed structs.
Such syntax feature was explicitly disabled, because the runtime interpreter did not have any information on the struct's contents, only about it's size. When the user's managed object is disposed, engine cannot unreference the pointers contained in such struct, which leads to nested objects never disposed, occupying program memory (sort of "memory leak").
With addition of RTTI it is now possible to gather information about pointers contained within a type, and correctly unref all them on type's instance's disposal.
This PR introduces new script op code:
SCMD_NEWUSEROBJECT2
, which has 2 args: typeid and size.The typeid is a local type id from the script's RTTI table.
Runtime interpreter converts local type id to a global one when creating a user object.
After loading scripts, engine generates a "flat" table of managed pointer offsets per struct. This speeds up user object disposal, as we don't have to analyze all the nested types all the time. Instead we do that only once the script is loaded, and then use a plain list of offsets.
Managed pointers in managed structs are enabled in both compilers, old and new one.
Both compilers declare extension "NESTEDPOINTERS", and a script author can use SCRIPT_EXT_NESTEDPOINTERS macro to test whether this feature is supported.
TESTING
The idea is that a structure of any complexity should be correctly disposed and removed from the memory when all references to it are gone. For example, you may have a managed struct that contains a particularly large dynamic array, and continuously create instances of that struct, observing program's memory usage.
ISSUES / TODO: