Skip to content

Commit d4d5534

Browse files
KhitiaraPerksey
Khitiara
and
Perksey
authored
Proposal - 3.0 Managed Chain Changes (#1791)
* Create Proposal - 3.0 Managed Chain Changes * Rename Proposal - 3.0 Managed Chain Changes to Proposal - 3.0 Managed Chain Changes.md * Update Proposal - 3.0 Managed Chain Changes.md --------- Co-authored-by: Dylan Perks <[email protected]>
1 parent 37add9a commit d4d5534

File tree

1 file changed

+225
-0
lines changed

1 file changed

+225
-0
lines changed

‎documentation/proposals/Proposal - 3.0 Managed Chain Changes.md‎

Lines changed: 225 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,225 @@
1+
# Managed Structure Chain Revisions for 3.0
2+
3+
## Summary
4+
An overhaul of the managed wrappers and helpers for the use of
5+
Vulkan structure chains.
6+
7+
# Contributors
8+
- [Khitiara](https://github.com/Khitiara)
9+
10+
# Current Status
11+
- [x] Proposed
12+
- [x] Discussed with API Review Board (ARB)
13+
- [x] Approved
14+
- [ ] Implemented
15+
16+
## Background
17+
18+
### The Underlying Vulkan API
19+
Ubiquitous in Vulkan are the use of "structure chains," a system
20+
whereby many Vulkan structs can form a singly linked list. This
21+
is used by the Vulkan API primarily to support extending core
22+
function calls with additional functionality, e.g. an
23+
`ImageCreateInfo` used to create a GPU-side image object may
24+
be extended with an `ImageFormatListCreateInfo` used to specify
25+
additional image formats, the latter type having been originally
26+
defined as `ImageFormatListCreateInfoKHR` in an extension before
27+
being promoted to core api surface. To implement this system,
28+
Vulkan structs which participate in the structure chaining
29+
system have some additional fields, presented here with C#
30+
pseudo-code.
31+
32+
```csharp
33+
public enum StructureType : int {
34+
// ...
35+
ImageCreateInfo = 14,
36+
// ...
37+
ImageFormatListCreateInfo = 1000147000
38+
// ...
39+
}
40+
41+
public struct ImageCreateInfo {
42+
[FieldOffset(0)]
43+
public StructureType SType = StructureType.ImageCreateInfo;
44+
[FieldOffset(4)]
45+
public void* PNext;
46+
// ...
47+
}
48+
```
49+
50+
This system also includes two bare-bones additional structures,
51+
`BaseInStructure` and `BaseOutStructure`, both containing only
52+
the `SType` and `PNext` members at the same offsets. Any `PNext`
53+
in a chain may be reinterpreted to one of those two types,
54+
`SType`inspected to determine the actual value type, and then
55+
the pointer reinterpreted to the appropriate complete structure
56+
type. The existence of both `BaseInStructure` and `BaseOutStructure`
57+
seems to exist to distinguish structs meant to be passed to or
58+
from vulkan, though no consistent guideline on which struct to
59+
use could be easily found leading the author of this proposal to
60+
surmise that the distinction may exist primarily as an
61+
implementation detail nonetheless exposed publicly.
62+
63+
### The Situation in Silk.NET 2.X
64+
65+
In Silk.NET 2.X, a number of utility types and methods are
66+
available to facilitate easier use of these structure chain
67+
types from managed C# code, where normal use would require
68+
an absurd amount of `fixed` statements or the use of unsafe
69+
bcl APIs. The available utility functionality in Silk.NET 2.X
70+
is broadly divisible into two distinct systems: a set of
71+
convenient wrappers over the unsafe ref-manipulation bcl
72+
APIs for common chain operations, and a set of managed
73+
`Chain<...>` types which perform manual memory allocation
74+
in a single unmanaged memory block where the structs are
75+
stored using `unsafe` pointer reinterpreting, and exposing
76+
properties and methods which allow the structure instances
77+
in a chain to be manipulated while keeping the chain of
78+
`PNext` pointers intact. These two systems are both built
79+
on a set of marker interfaces which are implemented on
80+
vulkan structure types by SilkTouch.
81+
82+
The wrapper functions over ref-manipulation, comprised mostly
83+
of extension methods on `ref T`, are unaffected by this
84+
current proposal, as the motivating issues are primarily
85+
design choices for the managed heap-safe `Chain<...>` types
86+
and for the marker interfaces which were made early in
87+
Silk.NET 2.X and are either not possible to change within
88+
the 2.X cycle due to the Silk.NET breaking change policy or
89+
which could be augmented by features in newer .NET versions
90+
which cannot be relied on by Silk.NET 2.X, such as static
91+
abstracts on interfaces. As Silk.NET 3.0 will target .NET 8
92+
and permits breaking changes for the major version rollover
93+
only, a number of improvements, both breaking and nonbreaking,
94+
may be made for this upcoming major version release.
95+
96+
## Prior Proposals
97+
- [Chain Polymorphism Proposal](https://github.com/dotnet/Silk.NET/blob/e8c0026fd2ebbc382b89b923b9bcaffc3ba1ede3/documentation/proposals/Proposal%20-%20Chain%20Polymorphism.md),
98+
proposed in [#1145](https://github.com/dotnet/Silk.NET/pull/1145).
99+
- [Addition of ref properties in #1420](https://github.com/dotnet/Silk.NET/pull/1420),
100+
amended in [#1423](https://github.com/dotnet/Silk.NET/pull/1423) due to errors in
101+
ref semantics.
102+
103+
## Proposed API Changes from 2.X
104+
~~Apologies for the lack of C# syntax highlighting, as
105+
highlighting the differences has been prioritized~~
106+
107+
```diff
108+
public interface IStructuredType
109+
{
110+
+ public static abstract StructureType StructureType { get; }
111+
+
112+
- public StructureType StructureType();
113+
+ public StructureType FixStructureType();
114+
}
115+
```
116+
The existing `IStructuredType.StructureType()` method sets the
117+
struct's `SType` field to be the correct value for the particular
118+
struct type and then returns it. It is proposed that the .NET 7+
119+
static abstracts in interfaces feature be used to force the
120+
presence of a static property to return the correct enum value
121+
for that type in a static context, and that the `StructureType()`
122+
method be renamed to suggest its updated purpose of correcting
123+
the `SType` field on the struct. The return behavior of the
124+
method is left as-is to permit ease of implementing the changes
125+
as much of the existing implementation relies on this behavior
126+
for concise implementation.
127+
128+
```diff
129+
public class Chain<TChain, T1, ...> : Chain
130+
{
131+
// ...
132+
- public unsafe BaseInStructure* Item1Ptr { get; }
133+
+ public unsafe T1* Item1Ptr { get; }
134+
// ...
135+
}
136+
```
137+
This change has been proposed previously but was rejected at the
138+
time as source-breaking changes during the 2.X cycle were not
139+
permitted. As Silk.NET 3.0 permits breaking changes from 2.X
140+
the change can now be made. Having readily accessible type-safe
141+
pointers helps a lot with much of how the `Chain<...>` types
142+
are commonly used.
143+
144+
This design also renames the non-generic `Chain` instead and gives it
145+
the head structure type `TChain` as a generic argument.
146+
```diff
147+
public abstract class BaseChain<TChain>
148+
{
149+
// ...
150+
- public abstract unsafe BaseInStructure* HeadPtr { get; }
151+
+ public abstract unsafe TChain* HeadPtr { get; }
152+
// ...
153+
}
154+
155+
public class Chain<TChain, ...> : BaseChain<TChain>
156+
{
157+
// ...
158+
- public override unsafe BaseInStructure* HeadPtr { get; }
159+
+ public override unsafe TChain* HeadPtr { get; }
160+
// ...
161+
}
162+
```
163+
164+
The reason for separating `BaseChain<TChain>` from `Chain<TChain>`,
165+
the chain of one element, in this design is that the latter
166+
performs management of unmanaged memory sized to hold the singular
167+
`TChain` object whereas the former is the base type of all chains
168+
starting with `TChain`, which is to say all chains before generic
169+
specialization, and has no inherent knowledge of the size of
170+
the allocated unmanaged memory.
171+
172+
```diff
173+
public class Chain<TChain, ...> : Chain
174+
{
175+
// ...
176+
+ public ref TChain GetPinnableReference();
177+
// ...
178+
}
179+
```
180+
Adding `GetPinnableReference()` with that precise name permits the use
181+
of the following relatively convenient syntax for converting a full
182+
`Chain<...>` to a pointer; providing this method is a common
183+
convention for memory-holding types in modern C# that this method
184+
should be provided despite the `Chain<...>` object already holding
185+
a reference to unmanaged memory and thus not needing to be pinned
186+
by the garbage collector.
187+
```csharp
188+
Chain<ImageCreateInfo> chain = ...;
189+
fixed(ImageCreateInfo* ptr = chain) {
190+
_vk.CreateImage(device, ptr, null, &image);
191+
}
192+
```
193+
194+
```diff
195+
public interface IChain<TChain, T1, ...>
196+
{
197+
// ...
198+
+ public unsafe TChain* HeadPtr { get; }
199+
// ...
200+
}
201+
```
202+
The `IChain` interfaces permit polymorphism of managed chain objects and were
203+
introduced by the [Chain Polymorphism Proposal](https://github.com/dotnet/Silk.NET/blob/e8c0026fd2ebbc382b89b923b9bcaffc3ba1ede3/documentation/proposals/Proposal%20-%20Chain%20Polymorphism.md),
204+
proposed in [#1145](https://github.com/dotnet/Silk.NET/pull/1145).
205+
Adding typed pointer members to the `IChain` interfaces may be done
206+
without regard to the design chosen for the type of `HeadPtr` in the
207+
concrete chain types thanks to explicit interface implementation in the
208+
case no change is made to that member, and is easily implemented in any
209+
case no matter if the concrete `ItemNPtr` members are changed to type-safe
210+
through the same mechanism of explicit interface implementation.
211+
212+
# Meeting Notes
213+
214+
## 19/11/2023
215+
216+
[Video](https://www.youtube.com/live/yXNDZDE3AHE?feature=shared&t=1221)
217+
218+
- We believe that the chain abstractions break down significantly anyway when used as an output with layers that can possibly inject additional PNexts that aren't reflected in these abstractions.
219+
- As a result, this chain abstraction only really works with inputs in every case.
220+
- UntypedHeadPtr being changed from BaseInStructure is contrary to this assumption.
221+
- Why is it IStructuredType instead of ITypedStructure? Changing this doesn't affect too much, but leaving it as is doesn't affect too much either.
222+
- 3.0 is the opportunity to fix long-standing issues like this as it's already massively breaking, so we can do silly changes like this.
223+
- IStructuredType is already broken with this proposal, so let's just rename it anyway.
224+
- We believe that IBaseStructure is probably the most sensible name given how Vulkan names this.
225+
- Everything else looks fine, approved notwithstanding the above.

0 commit comments

Comments
 (0)