shared_ptr for types

[caleridas]

For context, also refer to PR: #496 (for illustration, not for immediate merging)

The intent is to replace the current pattern of "copying + allocating type instances" all over the place, to just declare types "constant objects" that are not copied anymore, but just have their pointers copied (with reference counting).

The way this presently works:

• construct some type
• assign it to an instance of "port" to store it as input/output type for a node
• implicitly this calls the virtual "copy" method of the type to allocate a fresh instance of the same thing

The way it should work:

• construct some type once
• each port just takes the constructed instance and uses it

The PR should illustrate:

• what it would (roughly) look like; and on the face of it, the representational change is not huge, fairly mechanical unique_ptr -> shared_ptr replacement
• how many copies there are -- this is the second PR where I went to the exercise of removing "implicit copying" to force spelling out every individual instance of type copying

The PR in itself does not attempt to (substantially) reduce the number of copying yet. This would be achieved by replacing function call patterns of the form:

fn(const rvsdg::some_type & type) { .... do_something(type.copy()) ... }

with

fn(std::shared_ptr type) { ... do_something(type) ... }

which can be done on top of this PR, converting the individual use sites one-by-one. (Converting everything in one go creates a merge-conflict-nightmare IMHO). The "end state" would be reached when the "copy" method on types can be removed entirely.

There are various nuances not address in this PR, like having singleton instances for commonly used types (would be feasible, and result in appreciable reduction of copies already).

The main "concern" with this is that the work cannot be meaningfully finished without some API changes that affect downstream projects, so I would appreciate comments on this. I can think of various ways to provide compatibility adaptors to ease the API transition, but it will unfortunately not be entirely disruption-free.

[caleridas]

@phate @sjalander @haved should have tagged as RFC

[haved]

I think using shared objects for types is nice. I noticed when dumping RVSDG as a graph, including a graph of all types (see #481), that the type graph contains a lot of ptr, mem and bit32.

I support moving from unique_ptr to shared_ptr. For types like PointerType, I assume the constructors will be made private, and only a single global shared_ptr<PointerType> will be available?

What about aggregate types? Will there be a shared registry to avoid creating multiple instances of struct{bit32, bit32}, or is it ok to have multiple, as it is relatively rare (i.e., will pointer equality ever be assumed to be the same as type equality?)

[caleridas]

Yes, for pointer, mem, (common) bit types.... there would be singleton instances, constructors could be made private to enforce this.

Ensuring uniqueness for types might be possible, but is not an immediate goal. It would require a registry and "hash-ability" of all types to check against registry, so if desired that would rather be a follow-up step. I am not clear how complicated it might get to enforce "strict" equality -- it would require passing the registry around & looking up newly created types in various places in the code and so forth.

Equality comparison at present is: 1. check pointer equality, if equal then resolved. 2. test for semantic equality which I think is strictly an improvement not attempting for "optimality". I am not sure how the performance trade-off between the cost of maintaining a registry vs. getting rid of all semantic equality checks is in practice, without measuring I guess it could be either way.

[phate]

As commented below, all downstream dependencies are still in repository. Thus, the downstream dependencies should be a huge concern as they would be immediately affected by this change as well.

[caleridas]

Proposed transition strategy:

1. unique_ptr -> shared_ptr in type storage and "copy" method.

This changes the places that currently store a unique_ptr to shared_ptr, and also the return value of the copy method. This is a minor API change that slightly causes compile-breakage to downstream clients (HLS), but it is also really trivially fixed (just make the same trivial change for every user-defined type).

In principle the API change of the copy method could be delayed, or rolled out differently (i.e. adding a transitionary additional copy_shared method or so), if this creates problems for anyone.

2. (gradual) replace API methods that expect "const some_type&" as arguments at present to "const std::shared_ptr&".

Currently all of these methods need to copy the received type in order to store them anywhere. After this change, they can just receive the reference they received and store it, so this will step-by-step remove all copy operations.

The intent is here to also allow time for downstream to adapt and provide a transitionary period.

2a. singleton instances for "frequently used types"

should be self-explanatory, can happen in parallel/together with 2

3. (maybe needs discussion) remove or rename the "port" abstraction

I do not see much value in it, it is basically a container for a unique_ptr, and would become mostly synonymous with shared_ptr. If this is removed, it might however be neat to also change the signature of the "type" method of inputs/outputs so the shared_ptr is exposed (otherwise they are not shallow-copyable by simply copying the shared_ptr).

goal would be to also enforce at the interface level that no "hidden" type copying exists anymore

4. remove the type "copy" method

should be self-explanatory

In a follow-up could then try to add a "hash" method to types to try and uniquify instances that compare equal.

[phate]

The transition strategy sounds all reasonable and good to me. I just have a few comments:

1. All the downstream dependencies (e.g. HLS) are still in the repository. I have not started the process of eviction (yet). Thus, you should be fine by changing the interface, fixing all compiler errors, and the CI green-lighting everything. Nothing else to break as far as I know. I am in favor in directly changing the API of the copy methods.
2. The port abstraction is currently mostly a container for types, but sadly only mostly. It is also used for wrapping up some additional information for graph imports/exports. These are the only subclasses of the port class. In my opinion, it would be better to subclass the region argument and result class for this, put this information in there, remove the import/export ports and with it the entire port class.

Otherwise, nothing more to add from my side.

[caleridas]

1. sounds good, means that I can proceed assuming that any failure is going to be visible. However, I am still wondering about long-standing feature branches that may need adaptation.
2. also sounds like a good strategy, will do it this way

[caleridas]

this has also been implemented; there might be a refinement that I will propose later (add a "value-type"-like wrapper for std::shared_ptr), but other than that, this is done.