Celtoys / Remotery

Single C file, Realtime CPU/GPU Profiler with Remote Web Viewer
Apache License 2.0
3.03k stars 255 forks source link

WIP: Vulkan sampling support #256

Closed Valakor closed 5 months ago

Valakor commented 6 months ago

NOTE: This pull request includes a handful of fixes that aren't strictly related to Vulkan sampling which I'll list below. They could be pulled out into a separate change if required.

  1. _Fix Remotery_Destructor: Move clearing of g_Remotery to after graphics API-specific deletion/unbind calls, avoiding crashes because of unchecked access to gRemotery.
  2. _Fix RMT_ARCH_64BIT: Check __arm64__ as well._
  3. _Fix rmt_GetLastErrorMessage declaration: Enclose inside extern "C" block for C++ users._
  4. _Fix rmt_ScopedD3D12Sample: Remove extra parens in end-of-scope object declaration which confuse some compilers that think it's a function declaration (most vexing parse)._
  5. Fix Remotery.js deserialization of Uint64 and Sint64 properties: Remotery.c already converts these to rmtF64 prior to sending them over the socket.
  6. _Add support for thread names on MacOS via pthread_setname_np and pthread_getname_np._

This is a first-pass implementation of Vulkan sampling support for Remotery based on the D3D12 implementation. It currently expects a handful of things about the user's Vulkan environment that are likely more restrictive than necessary but make the implementation much simpler.

I'm open to discussion/feedback about this implementation!

dwilliamson commented 6 months ago

Matthew, this looks amazing. I've just ended my day so will go through all this first thing tomorrow morning. Thank you!

dwilliamson commented 5 months ago

What's left to pull this out of Draft?

Valakor commented 5 months ago

What's left to pull this out of Draft?

I think it might be worth discussing the API of rmt_BindVulkan and whether the user should provide function pointers to Remotery or whether Remotery should continue to look them up dynamically. I'm not 100% sure if the current implementation will work in cases where the user links to Vulkan statically; having the user provide the function pointers would allow Remotery to support a wider range of Vulkan implementations / environments. Unfortunately, doing this would probably require including Vulkan.h in the Remotery header to avoid super gross forward declarations which I wasn't sure was acceptable - it's a pretty big include and Remotery.h is currently fairly lightweight.

Edit: Somehow this whole time I missed that Remotery already has an answer to this problem for CUDA - declaring all the required function pointers as void* passed via a struct. It would be fairly simple for me to change the Vulkan code to something similar. I could also model the Vulkan error handling similarly to CUDA if requested by mapping Vulkan error codes to new rmtError codes.

It's probably also worth discussing whether it's worth relaxing the current Vulkan version / extension requirements. If you're fine with the current limitations then it could probably be merged in its current form and future changes could be taken to support older Vulkan implementations.

dwilliamson commented 5 months ago

I think it might be worth discussing the API of rmt_BindVulkan and whether the user should provide function pointers to Remotery or whether Remotery should continue to look them up dynamically. I'm not 100% sure if the current implementation will work in cases where the user links to Vulkan statically; having the user provide the function pointers would allow Remotery to support a wider range of Vulkan implementations / environments. Unfortunately, doing this would probably require including Vulkan.h in the Remotery header to avoid super gross forward declarations which I wasn't sure was acceptable - it's a pretty big include and Remotery.h is currently fairly lightweight.

Edit: Somehow this whole time I missed that Remotery already has an answer to this problem for CUDA - declaring all the required function pointers as void* passed via a struct. It would be fairly simple for me to change the Vulkan code to something similar. I could also model the Vulkan error handling similarly to CUDA if requested by mapping Vulkan error codes to new rmtError codes.

By static linking, I presume you mean platforms like iOS? This issue here seems to imply you can link statically and use vkGetInstanceProcAddr as well: https://github.com/KhronosGroup/MoltenVK/issues/1809

Also, do you have to pass the vkGetInstanceProcAddr function to rmt_BindVulkan? We manage to avoid this with OpenGL and wglGetProcAddress, but only really because each platform has its own OpenGL function getter.

It's probably also worth discussing whether it's worth relaxing the current Vulkan version / extension requirements. If you're fine with the current limitations then it could probably be merged in its current form and future changes could be taken to support older Vulkan implementations.

I think it's not worth worrying about. If anybody needs extra functionality and you personally do not need it right now, it's worth waiting until somebody adds a PR for it.

Valakor commented 5 months ago

By static linking, I presume you mean platforms like iOS? This issue here seems to imply you can link statically and use vkGetInstanceProcAddr as well: https://github.com/KhronosGroup/MoltenVK/issues/1809

Yeah there are a handful of edge cases related to statically linking Vulkan that I haven't tested, though my understanding is that dynamic linking is almost always preferable (or even required) on most platforms.

Also, do you have to pass the vkGetInstanceProcAddr function to rmt_BindVulkan? We manage to avoid this with OpenGL and wglGetProcAddress, but only really because each platform has its own OpenGL function getter.

In theory no, but I was trying to avoid having to re-implement something like the dynamic loader in Volk (see volkInitialize). Further, I think it's possible the user may have a "special" loader function that is not the one retrieved directly from the dynamic library, but I don't know how common that is in practice. I think it's overall a lot easier and less error prone for Remotery to be passed all the function pointers, but it does make initialization more complex for the user.

dwilliamson commented 5 months ago

That's pretty much what we do with OpenGL.

However, in the absence of a wide array of platforms to test this on, currently, I'm more than happy to accept a PR that requires you to pass Vulkan function pointers as part of rmt_BindVulkan.

That said: Volk is MIT licensed, so I would also have no issues if the function pointer loading code was copied over for just the functions we need. Reference would have to be given in the code, of course.

Another Edit: And while I'm here, the code as it stands is perfect for a PR anyway. It catches your use-case well and you have done the heavy lifting of the entire Vulkan port. If it doesn't meet another user's needs, we'll get a PR and anybody can work on updating it.

Valakor commented 5 months ago

I think I'll submit changes to have the user pass the function pointers. I realized when I was doing some final cleanup that many (all?) Vulkan implementations might expose function pointers for extensions that aren't enabled (or not even supported) by the implementation, and there's no way to check for extension enable state.

That might be fine if we just expect the user to have enabled the right extensions ahead of time, but unfortunately there's no way to tell whether they're using an extension or promoted feature, e.g. an EXT extension promoted to KHR or any extension promoted to core functionality in a later Vulkan version.

dwilliamson commented 5 months ago

The new fn registration looks good, we can make smoother API changes later if required, but I doubt we'll need it.

The only build failures are typical Github spurious fails that have nothing to do with the code.

This is some great work; thanks for pushing it through!