Refactor TypeDesc and StructTypeDesc storage

[ld-kerley]

This PR moves the TypeDesc storage away from static storage to a model where these objects are stored inside the GenContext object.

The TypeDesc objects are registered in the GenContext object, potentially per document load.

Struct unit tests added for validation.

Outstanding questions:

• Decide where the struct test should live.
  • Leave it in stdlib?
  • Create a new folder to test MaterialX features separately from the standard nodes?
• Keep the TypeDescStorage class as a separate object type or fold its contents inside the GenContext class itself - which is the only place its currently used. Now folded inside GenContext class
• Does registering a TypeDesc need to be made more thread safe, related, make a decision about what the behavior should be if the same type is registered more than once.
  • Just skip the registration if the type name is already registered.
  • Register the newly provided type in place of whatever type is already registered.
  • Compare the existing registered type to the new type and raise an error if they differ.

I'll take a pass over the documentation once the code has been reviewed.

[niklasharrysson]

This makes sense. The reason it's implemented this way now (using a std::string_view for the initialization/registration, which gets stored as a hash and a static name map) is because of the lack of a good class for string interning. If we can allow a dependency on OIIO, using ustring instead would be a nice improvement.

[ld-kerley]

I actually hit CI issues with static lib builds and the typenamemap being static - so I've got an upcoming patch that moves the typenamemap in to the TypeDescStorage as well.

I still think it's an interesting idea to start using OIIO::ustrings - it would be nice if there was a header only version of this we could adopt and not need full library dependency.

[lgritz]

Expressing it in a header that has no other dependencies -- that could maybe be arranged? But there's still a singleton at the heart of it, it can't be expressed as completely inline.

[niklasharrysson]

I'm curious why you need to store the struct members description on every port? Would it not be possible to find these from the TypeDesc?

[ld-kerley]

The TypeDesc object itself doesn't store the structure members (because we want to keep that small), but instead the struct members are stored in the TypeDescStorage object, but there is a need to access the struct members in MaterialXRenderGLSL/MSL modules, which don't have access to a GenContext. The compromise here is to pass them along on the port downstream - most of the time this will just be a nullptr if the port isn't a struct..

[niklasharrysson]

Ah I see! Would it be possible to instead store this pointer on the actual TypeDesc objects?

I suspect the reason you didn't do that in the first place (and instead used the _structIndex) was to keep the TypeDesc objects at 64-bits. But now we have crossed that barrier anyway and the TypeDesc objects are 80 bits. So if we change the uint16_t _structIndex to a pointer holding the struct description we get 128 bits instead (two full machine words), which probably is equal or even better for performance compared to 80.

[ld-kerley]

You're completely right - I was trying not to increase the size of TypeDesc, but if we can just store the pointer in the TypeDesc itself, then it feels more natural to me, and might completely do away with the secondary storage.

Let me take another pass and see how things shake out in that direction - thanks Niklas.

[ld-kerley]

I think it came out a lot cleaner - and certainly simplified the data storage. A number of other things might get even simpler if we could use a similar approach to store the name string with a pointer inside TypeDesc, instead of in the separate map... Ultimately I like the OIIO::ustring idea - but I think that needs to be a separate PR to work through what it means to add the dependency.

[niklasharrysson]

That's great to see it turned out well.

I agree a lot more could be simplified by having access to the type name from inside the TypeDesc. In fact I think that is a requirement, since it doesn't feel right to store the type name in various places in addition to the TypeDesc itself.

I couldn't resist to take a stab at this, so I've made a draft change list for storing a pointer to the name on the TypeDesc. This removed a lot of the issues with having to pass the context around just to find the type name.

That commit has been added here now. To be honest I didn't mean to push this commit to your branch - I ment to push this to my own fork of your branch, but that failed miserably, and apparently it was pushed here instead :) Sorry about that, but maybe that's a good thing as we can now review the merged result. And if this turns out to not be a good path forward we can always revert the commit.

[ld-kerley]

No worries - team work makes the dream work! Just ping me either here - or on slack - once you're done pushing to the PR - so we don't clobber each other.

[niklasharrysson]

Thanks! It seems my last build fix made the repo green again now. I'm done for today, so feel free to take back control. I'll ping you first if I have some other changes to push.

[niklasharrysson]

With the change to store type descriptions locally in GenContext, a lot of the API calls have been updated to pass in the context. Currently it's a bit random how it's passed in. Maybe the API would look more consistent if the context was always passed as first argument to the calls that need this?

[ld-kerley]

I'm happy to conform that, I was wary of making too many API changes all at once. Perhaps we get this update to land - and then I can take a look at a separate PR that makes the API feel more consistent. I think there's also scope to look at passing GenContext as a const GenContext& in more places too perhaps.

[ld-kerley]

Interested to hear from @jstone-lucasfilm what his preference is here. Defer API changes - or roll them all in to the same PR?

[niklasharrysson]

I was mainly referring to just the API functions that were changed here anyway. Not to change this in all API. That could definitely wait for a later PR.

But this was just an idea for consistency. Perhaps it becomes less consistent anyway because of how it is passed in other places in the API. It’s just coding style and not a big issue.

[ld-kerley]

I do like the idea of making it consistent as a secondary PR.

[niklasharrysson]

@ld-kerley This looks like a great direction to go, to remove the static storage of the type descriptions. I've added some initial comments above. Thanks!

[ld-kerley]

While we're considering larger scale changes - the other thing that would remove another slight "wort", would be if we switched the TypeDesc storage from an unordered_map to a container that retains order.

Before the structs it was unimportant, but because we're not allowing a struct to use another previously defined struct, it's important to retain the order in which the types are registered - at least the structs.

We could just go simple and use vector<TypeDesc>. I know we do lookup by name, but find() on a vector of approx 30 elements is not crazy. That or we could use map or maybe even tsl::ordered_map (https://github.com/Tessil/ordered-map) which I've used in other projects and found to be nicely performant - header only.

[niklasharrysson]

While we're considering larger scale changes - the other thing that would remove another slight "wort", would be if we switched the TypeDesc storage from an unordered_map to a container that retains order.

Before the structs it was unimportant, but because we're not allowing a struct to use another previously defined struct, it's important to retain the order in which the types are registered - at least the structs.

We could just go simple and use vector<TypeDesc>. I know we do lookup by name, but find() on a vector of approx 30 elements is not crazy. That or we could use map or maybe even tsl::ordered_map (https://github.com/Tessil/ordered-map) which I've used in other projects and found to be nicely performant - header only.

Since the GenContext is a big class and we normally have only a single instance of this per thread, I think it's fine to store extra data here, if needed for better performance. So we could store both a map and vector for the type descriptions, one for quick access by name and one for ordered access.

[ld-kerley]

As we're only storing the raw pointer here, aren't we relying on this TypeDesc object only ever being used inside the lifetime of its respective GenContext object? I'm not 100% sure on the life cycle of GenContext - but there are places where TypeDesc gets read from the ShaderPort in the MaterialXRenderXXX modules - is it possible that the GenContext object has been destroyed by that point? I also wonder if there are life-cycle issues if TypeDesc objects are passed across the python binding boundary - and used who knows when.

Wondering if instead it's safer to store a shared_ptr here? Which would let us do away with the string storage in GenContext too.... thoughts?

[niklasharrysson]

That's a great point Lee. I used a raw pointer to minimize the class size still, and my plan was to change _structMembers to a raw pointer too for the same reason. As you know, the original design of TypeDesc was a minimal size POD class that could be passed by value and stored by value everywhere in the API. So using smart pointers here is not ideal, as they both take more space and will need refcount increment/decrement for every pass and use of a TypeDesc.

But your are definitely right about the possible lifetime issues when the data is now stored in the GenContext.

I think we have two options:

1: We continue in this direction and let TypeDesc be a "large" class. It can hold all data needed internally and control lifetime of this data, adding smart pointers and all that. But then I think the current API design of passing and storing these by value everywhere would need to change as well. At least passing them by reference instead. And maybe also storing themselves as smart pointers everywhere, on the ShaderPorts etc.

2: We revert back to the original design where TypeDesc is a minimal class, and we keep all API unchanged in terms of passing and storing them. Large data like name strings and struct members needs separate storage then, but since the lifetime of a GenContex is not guaranteed the GenContex may not be the best place to store this. So I wonder if it would be better to revert back to a singleton class managing all TypeDesc storage after all, and instead take a different approach for making the data access thread safe.

I think I'm in favour of option (2) as that would be a much smaller update and no API would need to change. Instead of using thread local storage (GenContext) we could add some synchronization logic for accessing TypeDesc data in the singleton storage. The data can then be accessed from everywhere and we don't have to make the API changes to pass the GenContext around either.

Note that access to the standard data types has always been thread-safe, as their TypeDesc's are statically created. So it's only the dynamic creation of struct types that adds the need for some extra thread safety. Thread synchronization using std::mutex/std::lock_guard is already used elsewhere in MaterialX so perhaps that's something we could use here as well.

What do you think? If you agree it's not a bad idea I could try this out in another branch, to see how it works out in practice. If it works out well we could merge the best parts from both branches.

[ld-kerley]

I think it would be a little sad to reverse direction - though I think I agree of the two suggestions above (2) is the preferable one. If only because more API changes is going to make things harder to adopt outside MaterialX in downstream projects like USD etc.

I might throw out another suggestion I think "could" work.... let's call it (3) :)

3: Use raw pointers inside TypeDesc for storing name and struct members (to keep TypeDesc small). Then instead of trying to figure out how to deal with the lifetime issues outside of MaterialXGenXXX modules, how about we just make it private to the shader generation stage, thus making the GenContext storage adequate.

This would need a couple of changes:

• Removing TypeDesc from the python and javascript bindings - I think this is ok, and personally think we should be trying to reduce the binding interfaces to the "internals" of MaterialX, so make changes like this easier.
• Fixing the TypeDesc objects used in the MaterialXRenderXXX modules. It looks like we only use the TypeDesc object on the ports that are exposed as Uniforms in the HW shader render modules. So maybe we add shared_ptr storage for name and the struct members to ShaderPort, but leave them as nullptr for most of the time, and update them as we're finishing up shader generation, only for the uniforms and only for languages that need it (GLSL/MSL), and just redirect the raw pointers in TypeDesc to these new storage elements right at the end of the shader generation stage.

The other thing I'd mention. The string storage, I think, could be safely removed from GenContext. What we're doing really is equivalent to OpenImageIO ustring, in as much as we're using a hash of a string as a proxy for the string itself. Even if we have multiple shader generations happening simultaneously, using the same names, they are all going to evaluate to the same hash, so they could all share the same string table storage. Perhaps we need to make adding the strings thread safe, if the add is non-atomic. So if we do go down the (3) route, then we'd only need to add the storage pointer to ShaderPort for the struct members - and only ever need to update those in the rare case (?) they were in the Uniform block.

[niklasharrysson]

Thanks for those points Lee. There are certainly some advantages of letting the GenContext be the storage/maintainer of the type system. But there are also cons, and in particular two that I'm seeing. The first is that a lot of API changes are needed to pass around the GenContext everywhere in order to be able to query the types. And the second is the lifetime handling of the larger data. If TypeDesc's are used and needed after the lifetime of the GenContext it just doesn't feel right to let the GenContext be the owner of this data. Adding extra members to all ShaderPorts, in order to transfer this ownership in special situations, doesn't feel like a solid design to be honest. To my mind it's just so much cleaner to have a global type system, similar to what we have now, but make it thread safe to access everywhere. I think we can use a better separation of built-in types and custom/struct types in order to only need thread syncronization when custom types are queried, which is probably less than 1% of the cases.

I have a changelist in the works to better exaplain what I mean. Maybe we can take that discussion offline? I can ping you on Slack when I have something ready :)

Btw, your suggestion about trying to reduce our binding interfaces is something I agree on 100%.