Skip to content
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

WGPUCore working and refactor metaissue #18

Open
VarLad opened this issue Mar 18, 2024 · 22 comments
Open

WGPUCore working and refactor metaissue #18

VarLad opened this issue Mar 18, 2024 · 22 comments

Comments

@VarLad
Copy link
Member

VarLad commented Mar 18, 2024

I've been trying to go through the code of WGPUCore and its quite complex.
I think I don't understand the workings of a few functions.

For example, setting getDefaultBackendType to return WGPUBackendType_OpenGL doesn't seem to work at all.
I also don't understand the function of cstruct all around the code.

@arhik I'd like to help with the refactor, with some help from you.

@arhik
Copy link
Member

arhik commented Mar 18, 2024

Yeah its quite complex. Its not supposed to be. There could be better ways. But sure. I can help you. Its actually me who needs help and I would gladly accept any refactoring suggestions you can come up with.

I don't recall much of the code so I cannot help you with OpenGL backend issue right away. I will have to dig into it. There are few ugly globals which needs attention; that is all I remember.

I can help you with CStruct though. There are two reasons why CStruct exists:

  1. First reason being WGPUNative is mostly autogenerated using Clang.jl library; and most structs are bulky in the generated files. When developing interfaces for common utilities in WGPUCore, a naive approach will have to define upfront every field of the struct including nested struct fields. This was annoying.
  2. Second reason being, Clang autogenerates struct as immutable through out. So partial instantiation is not possible.

So Inshort CStruct is basically a way to set only fields we want to set by allocating struct sized memory using malloc. (We are also stepping out of garbage collection.) and computing the field offset in native struct. CStruct is basically an unsafe access to WGPUNative struct objects which can overwrite the fields with the values passed. Because modifying generated struct code to mutable is cumbersome for each release, I came up with a short cut means to use the generated code directly. There is some type checks in place but its rather unsafe still.

@arhik
Copy link
Member

arhik commented Mar 18, 2024

There is also third reason. This is an embarassing issue I struggled for months together. I struggle to keep object references alive consistently. GC kicks in randomly destroys few objects which are still needed. You can check test folder for hints on it. Fragmentstate, vertexBufferlayout, renderpipeline test files are always trouble makers for each minor change. Even now I disabled finalizer for CStruct. This means we are leaking memory. Luckily this doesn't happen in the while loop.

@VarLad
Copy link
Member Author

VarLad commented Mar 18, 2024

So Inshort CStruct is basically a way to set only fields we want to set by allocating struct sized memory using malloc. (We are also stepping out of garbage collection.) and computing the field offset in native struct. CStruct is basically an unsafe access to WGPUNative struct objects which can overwrite the fields with the values passed. Because modifying generated struct code to mutable is cumbersome for each release, I came up with a short cut means to use the generated code directly. There is some type checks in place but its rather unsafe still.

https://github.com/JuliaObjects/Accessors.jl seems like the best tool for this. I'd recommend checking out its documentation.

There is also third reason. This is an embarassing issue I struggled for months together. I struggle to keep object references alive consistently. GC kicks in randomly destroys few objects which are still needed. You can check test folder for hints on it. Fragmentstate, vertexBufferlayout, renderpipeline test files are always trouble makers for each minor change. Even now I disabled finalizer for CStruct. This means we are leaking memory. Luckily this doesn't happen in the while loop.

That might explain the random crashings we sometimes got while running the examples.
A goal of the refactor would definitely be to fix this!

@arhik
Copy link
Member

arhik commented Mar 18, 2024

I think I checked Accessors.jl. We also need a way to get pointer and create opaque structs. while cstruct |> ptr gets you pointer. pointer_from_objref doesn't work on immutable objects.

@arhik
Copy link
Member

arhik commented Mar 18, 2024

I am fairly certain that CStructs is not the issue, but you are more than welcome to prove me wrong.

@arhik
Copy link
Member

arhik commented Mar 19, 2024

By the way CStruct has performance issues. Most of the time is spent in offset computation. Statically baking offset info upfront would be ideal. It didn't matter much for initial prototyping but we can definitely improve on current CStruct approach.

@VarLad
Copy link
Member Author

VarLad commented Mar 20, 2024

@arhik We took a stab at trying to remove the global usages from the package for a few hours.
We couldn't deal with wgpuInstance.
The current structure of the library makes it incredibly hard to work with.

Would you be interested in rethinking the API? I was thinking about using https://github.com/MasonProtter/Bumper.jl massively to deal with the GC issues somehow.

@arhik
Copy link
Member

arhik commented Mar 21, 2024

Nice. Yeah. WGPUCore is a mess. Do you mind sharing what you tried ? My take on it is that you wouldn't need bumper.jl for wgpuInstance yet. Its very primitive ccall. And CStruct would be enough. Even if you want to static compile instance creation CStruct like approach is enough. Relying on Bumper.jl would only complexify things. CStruct is basically malloc underneath. By the way I wrote a slab allocator before Bumper.jl was announced and shelved it because it is not useful yet. I realized it will only complexify things. CStruct has simplest allocator. I would still like to take a look and understand where the problem is. I am currently refactoring WGSLTypes using WGPUTranspiler. It is a bit priority for me.

@VarLad
Copy link
Member Author

VarLad commented Mar 21, 2024

@arhik We simply removed wgpuInstance global variable and made it a parameter of the function, with default value as Ref{WGPUInstance}()
In effect should be the same thing, but I believe its getting deleted by the GC.

There's also a few bits in the Core library which we're not understanding (for example backendType being defined but not being used anywhere etc).
We've begun rewriting the WGPUCore library inspired by https://github.com/rajveermalviya/go-webgpu . At the current stage, we're trying to get the triangle example to run with the new abstraction.

We believe the abstractions there are a good model to follow. If you're interested, we can make the effort public.

If you've no issues with that, we think that the new API should live inside a new branch (lets call it "rewrite") in this repo, and should eventually replace the current API, after we're done adapting other repos to the new API.

Tell us what do you think.

@arhik
Copy link
Member

arhik commented Mar 21, 2024

Sure. Go ahead. rewrite branch is totally fine. But make small changes for each commit and make sure they are standalone. I will try to give my 2 cents on review.

I went through exhaustion of reiterating more than four times. So, I will have to recollect patience again to go through this code and contribute. Fresh eyes will definitely make difference.

Go will use defer. I remember trying that approach. I am not sure if I made commits along those lines. I will check.

@arhik
Copy link
Member

arhik commented Mar 21, 2024

You see stale code because of multi rewrites of API. Its common. I thought of writing a package that can check for stale code. So, I didn't bother to do manual work. Sometimes I also put placeholders.

@arhik
Copy link
Member

arhik commented Mar 21, 2024

@VarLad
Copy link
Member Author

VarLad commented Mar 23, 2024

@arhik One thing we're pretty confused about is:
https://github.com/rajveermalviya/go-webgpu/blob/main/wgpu/instance.go#L36-L37

Here, sType should be WGPUSType but whats provided (WGPUSType_InstanceExtras) is of WGPUNativeSType.

As such, when I do the same in Julia, i.e., write WGPUChainedStruct(C_NULL, WGPUSType_InstanceExtras), I get:

MethodError: Cannot `convert` an object of type WGPUNative.LibWGPU.WGPUNativeSType to an object of type WGPUNative.LibWGPU.WGPUSType

Closest candidates are:

convert(::Type{T}, !Matched::T) where T

@ Base Base.jl:84

I'm not quite sure whats going on here. Do you have any ideas?

@arhik
Copy link
Member

arhik commented Mar 23, 2024

We do explicit conversion I think.

@ut112002
Copy link
Member

Can you give me an example of how to do that ?

@arhik
Copy link
Member

arhik commented Mar 23, 2024

Both are enums. Ints are common type. So cast to int32 and back.

@arhik
Copy link
Member

arhik commented Mar 23, 2024

@arhik One thing we're pretty confused about is: https://github.com/rajveermalviya/go-webgpu/blob/main/wgpu/instance.go#L36-L37

Here, sType should be WGPUSType but whats provided (WGPUSType_InstanceExtras) is of WGPUNativeSType.

As such, when I do the same in Julia, i.e., write WGPUChainedStruct(C_NULL, WGPUSType_InstanceExtras), I get:

MethodError: Cannot `convert` an object of type WGPUNative.LibWGPU.WGPUNativeSType to an object of type WGPUNative.LibWGPU.WGPUSType

Closest candidates are:

convert(::Type{T}, !Matched::T) where T

@ Base Base.jl:84

I'm not quite sure whats going on here. Do you have any ideas?

I don't really know why that's the case. It was like that since first version of API we tried. May be file an issue. I wanted to but I found work around and forgot about it.

@arhik
Copy link
Member

arhik commented Mar 23, 2024

sType = WGPUSType(Int32(WGPUSType_DeviceExtras)),

like this

@arhik
Copy link
Member

arhik commented Mar 23, 2024

Don't bother with casting either. I wrote that to see explicitly. Just pass int. That works too. Its like inverse lookup.

sType = WGPUSType(Int32(WGPUSType_DeviceExtras)),

like this

@arhik One thing we're pretty confused about is: https://github.com/rajveermalviya/go-webgpu/blob/main/wgpu/instance.go#L36-L37
Here, sType should be WGPUSType but whats provided (WGPUSType_InstanceExtras) is of WGPUNativeSType.
As such, when I do the same in Julia, i.e., write WGPUChainedStruct(C_NULL, WGPUSType_InstanceExtras), I get:

MethodError: Cannot `convert` an object of type WGPUNative.LibWGPU.WGPUNativeSType to an object of type WGPUNative.LibWGPU.WGPUSType

Closest candidates are:

convert(::Type{T}, !Matched::T) where T

@ Base Base.jl:84

I'm not quite sure whats going on here. Do you have any ideas?

I don't really know why that's the case. It was like that since first version of API we tried. May be file an issue. I wanted to but I found work around and forgot about it.

next object is null. So why bother setting it.

@arhik
Copy link
Member

arhik commented Mar 31, 2024

@VarLad @ut112002 Please check new WGPUNative release. Now we dont have to use CStruct. Also the interface can get closer to go version. I didn't register the updated package yet for you to take a look at it and comment on it.

@arhik
Copy link
Member

arhik commented Apr 3, 2024

We have a static compileable example in WGPUNative.jl now. Only works on Mac M series I think. I tried it on windows and linux with no luck. static compileabel example

@arhik
Copy link
Member

arhik commented Apr 3, 2024

I couldn't adapt path to user location. So you need to set path to path = WGPUNative.LibWGPU.libWGPU like I did.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants