Nested circular references using load_and_allocate #44
Comments
|
Can you see if that fixes it? I can't test VS2013 until tonight, but it looks like I just made a really small typo for load_and_allocate which wasn't in the normal version. |
|
I'll check in 3 hours! Thanks. Sent from my iPhone On 2014-01-16, at 2:38 PM, Shane Grant notifications@github.com wrote:
|
|
So while my quick fix was relevant there is still an issue with this that I'll have to fix which is a bit more difficult to tease out. It only happens with the The problem is that let's say you have a shared_ptr to some struct that contains a nested circular reference to itself as a member variable. When we do the load, we jump into load_and_allocate because there is no default constructor. You would likely write something like: struct MyStruct
{
MyStruct( std::shared_ptr<MyStruct p );
std::shared_ptr<MyStruct> ptr;
template <class Archive>
static MyStruct * load_and_allocate( Archive & ar )
{
std::shared_ptr<MyStruct> ptr;
ar( ptr );
return new MyStruct( ptr );
}
};
// serializing something like the following:
std::shared_ptr<MyStruct> ptr;The problem here is that since we need to defer the nested cirular load until the parent one is done, internally we are holding onto a reference to the local variable |
|
I see the difficulty. If we were to call something "like" serialize after load_and_allocate as a guaranteed post-load_and_allocate step, and perform the shared_ptr ptr; load in there would that solve the problem? I know originally I was trying to use load_and_allocate because I had no default constructor, but I was supplying nullptr to my shared_ptr parameter, and other "default" parameters (basically creating an object temporarily with safe junk parameters), and then calling serialize after to load in the real values as a post-load step. I had to modify cereal to always call serialize after load_and_allocate, however. This keeps the bulk of the loading code in one place (it's kind of goofy using load_and_allocate and having a different path for saving in serialize when the loading aspect of serialize was never used anyway.) But it does impose some class design issues (you need your constructors to be safely initialized with junk.) I didn't want to introduce an actual default constructor because this is really an implementation detail of cereal, not something I want to expose to users, even in a private way. This is why I didn't just create a default constructor to begin with. As long as you provide a guaranteed path to perform post-load_and_allocate pointer fixing, however, I think it should be fine. Can you make it call something like "load_circular_references" or something in a post step? I haven't thought this out enough to know if you'd end up with troubles if you have multiple nesting by doing that though. Let's say you had: A -> B -> B + A (where B contains a circular reference back to itself and also to A) would it go through the proper load_and_allocate path and then load_circular_references at a safe time as well? |
|
There are some things you cannot reasonably do during construction, and this is kind of related to trying to call shared_from_this() in a constructor. Often the work-around is really just doing a factory construction and supplying the shared_ptr() to the dependent children after creating the parent. In this case we change scopes a few times though so it's harder to map conceptually, but the problem is similar. (random thought) |
|
Mentally I was trying to figure out if there's a way to provide a kind of "future" to the shared_ptr which gets its value when the object loads. But I don't think it can be done without modifying the class constructor to take that type anyway. Maybe you could create some cereal_shared_ptr type to replace shared_ptr in serializable classes. This is kind of stretching, but is another alternative (kind of invasive though). |
|
Going to think about this for another day before doing anything. Fundamentally the way load_and_allocate works right now I don't see a way around having a two part variant of it, so the entire mechanism may need to change. |
|
Fair enough! Man, I feel kind of bad stirring up all this shit lol. If it's any consolation, I seem to have gotten the library to work for my case! But now that just means I'm unblocked so I'll keep implementing save/load stuff in all my major classes. (Albeit with public default constructors in my objects which I DO NOT want to have to keep if I were to open source this library (which I would like to do). With one developer it doesn't matter as much. |
|
Great work so far, thank you so much. |
|
Just curious if you've given this any thought. I'm encountering quite a few subtle issues in my code as well, trying to tease them out. It's pretty hard in general, but I feel like i'm getting very close. For example, I can fully save and load a single node in my scene graph. I'm now beginning to have troubles with multiple layers, in some instances my textures are somehow decrementing a ref count one more time than they should which causes a crash! I honestly don't even know how that could reasonably occur considering the nature of a shared_ptr (increment on create, decrement on delete... How would I have one more delete than create?) Unfortunately I'm having troubles reproducing this in a smaller simple example with no dependence on my own code (this is exceptionally troublesome, there is something subtle in play here.) I can hook you up with the repository if you're curious, but I'll bash against it for a couple more days before I cede. Maybe I could offer you a small payment for assistance resolving my issue? Just let me know. :) |
|
I'd like to clarify that I do not see these same crash bugs without cereal load/save so it's something in the loading process, but maybe it is my use of your system, or maybe it's cereal itself getting a bit over-eager with the shared_ptr releases in some deeply nested edge case. |
|
There's another thing I'd like to mention: cereal doesn't do well with members that are references. Sometimes though, you may be interested in reconstituting a reference manually, but you need to supply it during the construction stage. It would be nice if you could somehow optionally bundle raw pointers and references in some kind of "storage" for the archive call, so you could query those values and then supply them in the constructor. IE: And then when loading: Just a thought, kind of related to re-thinking load_and_allocate. |
|
I suppose that kind of mechanism could be supplied as a service, but I would prefer dependency injection if possible (instead of making some global access to ping from within the cereal loading process.) |
|
I don't think we officially support references right now, though that isn't said anywhere. References kind of have exactly the same problems as raw pointers in terms of tracking, which wasn't something we set out to handle with cereal. Could what you proposed work properly without us keeping track of every memory address we serialize? If you want cereal to load things into memory and return references to them, you would also need for cereal to keep that data alive long enough for you to use it, which would mean it persisting past the destruction of the archive, which means it would most likely result in a leak. I haven't tried anything out yet but I'm floating around a solution for this with placement new that would have cereal allocating memory before entering into some kind of a load_and_allocate function which might be able to address some of the problems here. Essentially I want the shared_ptr to be set up and valid so that pointers and reference counts can be kept properly, with the actual data being modified only by the load and allocate function. |
|
I wouldn't expect it to handle reference lifetime, that would have to be IE: If your clickable object requires a reference to some mouse handler, And I look forward to hearing more about your ideas when you get around to On Mon, Jan 20, 2014 at 5:46 PM, Shane Grant notifications@github.comwrote:
|
|
Back to the crash Issue I mentioned, I'm currently investigating in more detail. Here's what I've figured out so far: my texture definition with two texture handles pointing to it is being deallocated during the load process. Here's what my scene graph looks like: http://m2tm.net/RandomImages/TestSceneProblem.png I'll be trying to create a minimal test case based on this information. |
|
I hope I'm not bugging anyone with spammy messages. I've managed to get this up and running without trouble. This should be approximately what my graph above represents in the trouble node. This means it's likely something I am doing, but I will update again if I find an issue with cereal instead (at least this narrows my search!): |
Won't have time to finish this one right now, but this solution should work well. 1) Realized there was no reason to do the deferring thing, we just register immediately after allocation and use that pointer, so got rid of some overhead in code/time on regular shared_ptr loads. 2) For cases with no default constructor the interface will be changing slightly. Instead of having the user do both the allocation and initialization, the user will only be responsible for the initialization. This works as follows: We allocate std::aligned_storage big enough for the type and pack it into a shared_ptr for the proper type with a custom deleter to correctly call the proper destructors. We'll wrap up this raw pointer in a new class called cereal::allocation (or similar) which will now be passed along with the archive as a reference to the load and allocate function. The user now loads up their data as before using the archive, and then instead of performing a raw call to operator new, they pass all of their arguments to the operator() of the cereal::allocation object which then performs a placement new into the aligned_storage (transparent to the user). Should resolve the circular reference problem too.
|
@m2tm, thinking without using my brain right now, would std::reference_wrapper help fix your issue with references? |
(RAMBLY TIRED THOUGHTS:) On to the second bit: I typed a lot, erased a lot, and now I have a clear idea. Cereal is very good at loading a closed circuit of data. If you consider everything could be owned by shared/weak_ptrs then cereal will diligently reconstitute those objects with new instances when you ask it to. Where this breaks down is when I have a reference from outside the scope of an archive. If I don't want to save out my renderer, or my mouse state handler, or some actor controller, or a root texture manager it all becomes a lot of work after the load to parse through and revisit nodes and set up links as they should be, but if I could supply this stuff in a way that didn't require global access, that would be nice. For example, I have a root Game class which creates regular variables for my various managers and such: Cool! The main issue here is that I don't really have a way of reconciling what goes on in the loading process with these objects. My Game class also has a scene: std::shared_ptrMV::Scene::Node mainScene; It's pretty easy to, after load, just call mainScene->setRenderer(renderer); and let that fucker go through all its children and propagate the change. The one main issue here is that I don't want my mainScene to be construct-able without a renderer in the first place, so in situations where something might query the size of the window, for example, suddenly I need to be very cautious about things that I usually would just implement in an RAII way. I now have to carefully ensure no renderer calls happen in the construction of a scene node of any type. If we want to snap a scene node to one side of the screen because it's anchored, now I have to somehow call that in a post step (or when I call setRenderer), it becomes hairy though when you have to consider this for every kind of situation. That's for renderer which is common to all Scene::Node objects. What about the mouse handler? This is a reference ONLY clickable nodes need. So how do I re-constitute that? I have three options (1 and 2 are inverse of each-other):
What I really want to be able to do is just say "okay, well, we already constructed these objects ahead of time, outside of the archive process, I just want to expose those to the archive so during the course of loading, my objects can pull those references and pointers and reconstitute themselves in one step instead of in many." Hopefully this describes the issue a bit better. I think it's easy to just say "we don't handle references and pointers", but realistically there's not a good way of reconciling anything saved directly in the archive with things that are not. This is all stuff I can work around, I'm mostly bringing it up because it kind of struck me as something that cereal could probably provide to close the loop on that construction issue. |
Passing tests but need to look over this with valgrind some more. Potentially have some issues here, moreso with unique_ptr than shared_ptr.
|
I see your point - let's break off this discussion into a new issue. |
Need to do a once over on documentation to finish up, see issue #22
|
Cool. I'll make a new issue that should be marked as an enhancement and prioritized however you see fit. Thanks for your attention on this bug! |
I'm opening this as a separate issue as it was introduced by the most recent couple check-ins on the develop branch. You should be using Visual Studio 2013 to reproduce this (I am currently on the November Compiler Prerelease as well, but I suspect it's a problem with the standard install too.)
The error I get is related to the line: if( ar.isSharedPointerValid ) in memory.hpp:
The following is what I get from my LIVE CODE, not from the above test. I'll go ahead and get the error for the above code this evening.
The text was updated successfully, but these errors were encountered: