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

Minor debugging enhancements. #2317

Merged

Conversation

billhollings
Copy link
Contributor

  • On GPU error, log indicate whether device has been resumed or lost, and report as either warning or error, respectively.
  • On VkDevice loss, stop device-scope GPU capture.
  • In debug mode, include address of Metal object in Metal object label.
  • Replace setLabelIfNotNil() with setMetalObjectLabel() and move inside MVKVulkanAPIObject to allow access to config info.
  • When logging contents of MVKDescriptorPool, only list descriptor types with allocations.
  • Add @autoreleasepool around complex NSString creation.

Expansion and replacement of PR #2305.

@billhollings billhollings mentioned this pull request Aug 27, 2024
@js6i
Copy link
Collaborator

js6i commented Aug 27, 2024

Thanks for pushing this forward, I was out last week and didn't get to it! I definitely glossed over the label part in the original submission.
It would be convenient to have an autoreleasepool assigned to the Metal command buffer, put the strings there and drain it on completion, but I can't find a way to do it..?

@cdavis5e
Copy link
Collaborator

Thanks for pushing this forward, I was out last week and didn't get to it! I definitely glossed over the label part in the original submission. It would be convenient to have an autoreleasepool assigned to the Metal command buffer, put the strings there and drain it on completion, but I can't find a way to do it..?

The @autoreleasepool block is just syntactic sugar for the NSAutoreleasePool object:

{
    NSAutoreleasePool *pool = [[NSAutoreleasePool alloc] init];
    // contents of block...
    [pool release];
}

For a long time creating an NSAutoreleasePool object explicitly was the only way to create an autorelease pool.

NSAutoreleasePool has a -drain method that releases all objects registered with the pool, without destroying the pool object itself. The intended use case is for a loop, like the main run loop of an application. Instead of creating and destroying an autorelease pool on every iteration, you create an autorelease pool before the loop, call -drain at the end of every iteration, and then, when the loop terminates, you release the pool. Theoretically, it shouldn't be difficult to adapt this to a command buffer; the one problem is that autorelease pools exist in a stack, and creating a new pool pushes it onto the stack and causes all calls to -[NSObject autorelease] to go to that pool. But, it turns out, NSAutoreleasePool also has explicit -addObject: and +addObject: methods. All -[NSObject autorelease] actually does is call +[NSAutoreleasePool addObject:], which in turn, invokes -[NSAutoreleasePool addObject:] on the pool on top of the stack. Any objects we create, then, could be explicitly placed in the autorelease pool associated with a command buffer. But it won't help with other frameworks and dylibs that call -[NSObject autorelease]. For that, the only thing I could think of is "swizzling"* +[NSAutoreleasePool addObject:] so that, if we're currently encoding or executing a particular command buffer, the call to -[NSAutoreleasePool addObject:] goes to the pool associated with that command buffer.

Anyway, this all sounds really complicated, and I'm not sure it's worth it, and why did I write all this again?

* Replacing a method's IMPlementation with one of our own. You use class_getClassMethod() or class_getInstanceMethod() on the class and selector of the method you want to replace; then you optionally save the old IMP with method_getImplementation() and then insert the new one with method_setImplementation(). If you want to use a closure block instead of writing out a function with the signature void (id, SEL, id), you can use imp_implementationWithBlock() to produce an IMP from the block.

@js6i
Copy link
Collaborator

js6i commented Aug 27, 2024

For that, the only thing I could think of is "swizzling"* +[NSAutoreleasePool addObject:] so that, if we're currently encoding or executing a particular command buffer, the call to -[NSAutoreleasePool addObject:] goes to the pool associated with that command buffer.

Anyway, this all sounds really complicated, and I'm not sure it's worth it, and why did I write all this again?

  • Replacing a method's IMPlementation with one of our own. You use class_getClassMethod() or class_getInstanceMethod() on the class and selector of the method you want to replace; then you optionally save the old IMP with method_getImplementation() and then insert the new one with method_setImplementation(). If you want to use a closure block instead of writing out a function with the signature void (id, SEL, id), you can use imp_implementationWithBlock() to produce an IMP from the block.

I was thinking of just using the pool for these label strings, and maybe some other selected allocations (not sure if there are any that would benefit from it). We could addObject: them if we allocate the string with something that doesn't put it in the pool that's currently on top of the stack, or maybe just retain, as there's no pushPool/popPool methods from what I could tell to guide where the autorelease objects go.
I'm assuming the lifetime of these labels should be at most the same as command buffer's*, and keeping a pool seems better than e.g. piling callbacks for each allocated string.. not ideal but good enough for a debug path.

  • And that's only right for some of the objects.. I think we could limit the unique names for things for which this is true, they're mainly useful for encoders anyway.

- On GPU error, log indicate whether device has been resumed or lost,
  and report as either warning or error, respectively.
- On VkDevice loss, stop device-scope GPU capture.
- In debug mode, include address of Metal object in Metal object label.
- Replace setLabelIfNotNil() with setMetalObjectLabel() and move
  inside MVKVulkanAPIObject to allow access to config info.
- When logging contents of MVKDescriptorPool,
  only list descriptor types with allocations.
- Add @autoreleasepool around complex NSString creation.
@billhollings
Copy link
Contributor Author

billhollings commented Aug 27, 2024

The NSAutoreleasePool hierarchy is inherently tied to a thread, so it's not clear that we could use that for a particular object, like the command buffers or encoders. If they are accessed only from single thread, we can just wrap a higher calls in @autoreleasepool. If they're accessed from multiple threads, we could use a basic NSMutableArray that we release when the main object is released.

I've pushed an update that eliminates all of the @autoreleasepools that I had added, because I realized that almost all of them were already covered by @autoreleasepools higher in the call stack, and where I wasn't sure, I added pools much higher up.

Hopefully that covers what we need here, and we won't need to introduce anything more complicated.

@billhollings billhollings merged commit 03f2896 into KhronosGroup:main Aug 28, 2024
6 checks passed
@billhollings billhollings deleted the minor-debugging-enhancements branch August 28, 2024 19:03
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

Successfully merging this pull request may close these issues.

3 participants