Fix #108792: Ensure Metal buffers correctly freed on exit #108940

Jason Fielder · 2023-06-13T16:29:53+02:00

Jason Fielder commented

2023-06-13 16:29:53 +02:00

Replaces vector of allocations with dynamic linked list.
Bug caused by previously freed buffers still having been in the
list. Linked list enables fast removal of already-released buffers.

Also ensured that the memory manager classes are included in
memory tracking.

Authored by Apple: Michael Parkin-White

Replaces vector of allocations with dynamic linked list. Bug caused by previously freed buffers still having been in the list. Linked list enables fast removal of already-released buffers. Also ensured that the memory manager classes are included in memory tracking. Authored by Apple: Michael Parkin-White

Jason Fielder added 1 commit 2023-06-13 16:29:59 +02:00

Fix #108792 : Ensure Metal buffers correctly freed on exit a0ce3d7141

Replaces vector of allocations with dynamic linked list.
Bug caused by previously freed buffers still having been in the
list. Linked list enables fast removal of already-released buffers.

Also ensured that the memory manager classes are included in
memory tracking.

Authored by Apple: Michael Parkin-White

Jason Fielder requested review from Sergey Sharybin 2023-06-13 16:30:07 +02:00

Jason Fielder requested review from Jeroen Bakker 2023-06-13 16:30:17 +02:00

Sergey Sharybin requested changes 2023-06-13 17:28:18 +02:00

Sergey Sharybin left a comment

Bug caused by previously freed buffers still having been in the list.

I am a bit confused. There was no lists before.

Do you mean that previously it was possible to have situation when some elements of vectors are freed, but not null-ed in the vector or something (leading to attempt to free them again)?

> Bug caused by previously freed buffers still having been in the list. I am a bit confused. There was no lists before. Do you mean that previously it was possible to have situation when some elements of vectors are freed, but not null-ed in the vector or something (leading to attempt to free them again)?

source/blender/gpu/metal/mtl_memory.mm Outdated

						
				@ -456,0 +516,4 @@

				    BLI_assert(prev == nullptr);

				  }

				  allocations_list_size_--;

				  BLI_assert(allocations_list_size_ >= 0);

Sergey Sharybin commented

2023-06-13 17:27:45 +02:00

allocations_list_size_ is unsigned, so the check is always true. You can do something like

BLI_assert(allocations_list_size_ >= 1);
allocations_list_size_--;

`allocations_list_size_` is unsigned, so the check is always true. You can do something like ``` BLI_assert(allocations_list_size_ >= 1); allocations_list_size_--; ```

Michael Parkin-White commented

2023-06-13 17:32:43 +02:00

First-time contributor

Bug caused by previously freed buffers still having been in the list.

I am a bit confused. There was no lists before.

Do you mean that previously it was possible to have situation when some elements of vectors are freed, but not null-ed in the vector or something (leading to attempt to free them again)?

Yes, by "list" I was referring to the original vector of elements. There was an erroneous situation after a recent change introducing memory cleanup which did not nullify the stored element in vector leading to a double free.

Replaced the vector with a linked list to prioritize fast insertion and removal, as the list is only ever iterated upon cleanup during shutdown.

(Will also update or just remove the allocations list size check as it is redundant. A number of the assertions are perhaps a little over-defensive given the usage of the code is rather minimal.)

> > Bug caused by previously freed buffers still having been in the list. > > I am a bit confused. There was no lists before. > > Do you mean that previously it was possible to have situation when some elements of vectors are freed, but not null-ed in the vector or something (leading to attempt to free them again)? Yes, by "list" I was referring to the original vector of elements. There was an erroneous situation after a recent change introducing memory cleanup which did not nullify the stored element in vector leading to a double free. Replaced the vector with a linked list to prioritize fast insertion and removal, as the list is only ever iterated upon cleanup during shutdown. (Will also update or just remove the allocations list size check as it is redundant. A number of the assertions are perhaps a little over-defensive given the usage of the code is rather minimal.)

Jeroen Bakker requested changes 2023-06-14 08:41:59 +02:00

source/blender/gpu/metal/mtl_memory.hh Outdated

						
				@ -108,2 +108,4 @@

				class MTLBuffer {

				 public:

				  gpu::MTLBuffer *next, *prev;

Jeroen Bakker commented

2023-06-14 08:35:05 +02:00

use ListBase, ListBaseWrapperTemplate and the functions in BLI_listbase.h. Some casts might be needed, but it reduces code-duplication.

use `ListBase`, `ListBaseWrapperTemplate` and the functions in `BLI_listbase.h`. Some casts might be needed, but it reduces code-duplication.

Michael Parkin-White commented

2023-06-19 15:28:33 +02:00

First-time contributor

Thanks, have modified the implementation. Admittedly not 100% sure if there are any catches with ListBase.
I have attempted to utilise both BLI_freelinkN for deletion and BLI_freelistN for complete freeing of the list, but a little unsure how the builtin MEM_freeN functions work alongside C++ new/delete, and with constructors.

i.e. is it valid for gpu::MTLBuffer to be created using new gpu::MTLBuffer(params...), and freed using MEM_freeN(buffer)?

It appears to be releasing the resources correctly, as it looked like it was sufficient to use MEM_freeN based functions, if you have MEM_CXX_CLASS_ALLOC_FUNCS specified, however, it would be very helpful if someone could confirm whether the new implementation in the latest commit is correct.

Cheers!

Thanks, have modified the implementation. Admittedly not 100% sure if there are any catches with ListBase. I have attempted to utilise both `BLI_freelinkN` for deletion and `BLI_freelistN` for complete freeing of the list, but a little unsure how the builtin `MEM_freeN` functions work alongside C++ new/delete, and with constructors. i.e. is it valid for gpu::MTLBuffer to be created using `new gpu::MTLBuffer(params...)`, and freed using `MEM_freeN(buffer)`? It appears to be releasing the resources correctly, as it looked like it was sufficient to use `MEM_freeN` based functions, if you have `MEM_CXX_CLASS_ALLOC_FUNCS` specified, however, it would be very helpful if someone could confirm whether the new implementation in the latest commit is correct. Cheers!

antonheg commented

2023-06-16 11:49:54 +02:00

First-time contributor

@Jason-Fielder, @Sergey, @Jeroen-Bakker, could we please have this fix merged as is, just to avoid those nasty random crashes and let @Michael-Parkin-White-Apple sort out the refactoring later on? 🙏

Update: I ended up building Blender from the main repo and applying the diff patch and as far as I am concerned (#108836) @Michael-Parkin-White-Apple 's fix does solve the crash on exit after a long sculpting session. 👍

@Jason-Fielder, @Sergey, @Jeroen-Bakker, could we please have this fix merged as is, just to avoid those nasty random crashes and let @Michael-Parkin-White-Apple sort out the refactoring later on? 🙏 Update: I ended up building Blender from the main repo and applying the diff patch and as far as I am concerned (#108836) @Michael-Parkin-White-Apple 's fix does solve the crash on exit after a long sculpting session. 👍

Chris Blackbourn referenced this pull request

2023-06-18 06:37:29 +02:00

Crash on exit with Metal backend #108792

Chris Blackbourn commented

2023-06-19 04:02:40 +02:00

Out of an abundance of caution, I have the PR applied and am reporting the following warning on exit:

Blender(81644) MallocStackLogging: attempting to over-release a stack. Potential double free bug.
Saved session recovery to "/var/folders/5j/z5m41t610_s4hwj4vrcx4ttr0000gn/T/quit.blend"

Blender quit
2023-06-19 13:59:04.470923+1200 Blender[81644:8353398] [client] No error handler for XPC error: Connection invalid
Program ended with exit code: 0

Out of an abundance of caution, I have the PR applied and am reporting the following warning on exit: ``` Blender(81644) MallocStackLogging: attempting to over-release a stack. Potential double free bug. Saved session recovery to "/var/folders/5j/z5m41t610_s4hwj4vrcx4ttr0000gn/T/quit.blend" Blender quit 2023-06-19 13:59:04.470923+1200 Blender[81644:8353398] [client] No error handler for XPC error: Connection invalid Program ended with exit code: 0 ```

👀 1

Michael Parkin-White commented

2023-06-19 11:32:35 +02:00

First-time contributor

Out of an abundance of caution, I have the PR applied and am reporting the following warning on exit:

Blender(81644) MallocStackLogging: attempting to over-release a stack. Potential double free bug.
Saved session recovery to "/var/folders/5j/z5m41t610_s4hwj4vrcx4ttr0000gn/T/quit.blend"

Blender quit
2023-06-19 13:59:04.470923+1200 Blender[81644:8353398] [client] No error handler for XPC error: Connection invalid
Program ended with exit code: 0

Thanks for raising this, I'll have a deeper look into this issue and see if there are any problems.
I'm unsure if this over-release is related. Was this also happening without the patch?

> Out of an abundance of caution, I have the PR applied and am reporting the following warning on exit: > ``` > Blender(81644) MallocStackLogging: attempting to over-release a stack. Potential double free bug. > Saved session recovery to "/var/folders/5j/z5m41t610_s4hwj4vrcx4ttr0000gn/T/quit.blend" > > Blender quit > 2023-06-19 13:59:04.470923+1200 Blender[81644:8353398] [client] No error handler for XPC error: Connection invalid > Program ended with exit code: 0 > ``` Thanks for raising this, I'll have a deeper look into this issue and see if there are any problems. I'm unsure if this over-release is related. Was this also happening without the patch?

Michael Parkin-White added 1 commit 2023-06-19 11:37:13 +02:00

Merge branch 'blender-v3.6-release' into Fix_108792 a8438137fd

Chris Blackbourn commented

2023-06-19 12:40:33 +02:00

Out of an abundance of caution, I have the PR applied and am reporting the following warning on exit:
Blender(81644) MallocStackLogging: attempting to over-release a stack. Potential double free bug.
Saved session recovery to "/var/folders/5j/z5m41t610_s4hwj4vrcx4ttr0000gn/T/quit.blend"

Blender quit
2023-06-19 13:59:04.470923+1200 Blender[81644:8353398] [client] No error handler for XPC error: Connection invalid
Program ended with exit code: 0
Thanks for raising this, I'll have a deeper look into this issue and see if there are any problems.
I'm unsure if this over-release is related. Was this also happening without the patch?

I have a suspicion the "Potential double free bug" is actually related to a different bug I was chasing today ( #107500 ), but I never previously saw the warning because of the metal-buffers issue.

... most likely nothing to see here ...

> > Out of an abundance of caution, I have the PR applied and am reporting the following warning on exit: > > ``` > > Blender(81644) MallocStackLogging: attempting to over-release a stack. Potential double free bug. > > Saved session recovery to "/var/folders/5j/z5m41t610_s4hwj4vrcx4ttr0000gn/T/quit.blend" > > > > Blender quit > > 2023-06-19 13:59:04.470923+1200 Blender[81644:8353398] [client] No error handler for XPC error: Connection invalid > > Program ended with exit code: 0 > > ``` > > Thanks for raising this, I'll have a deeper look into this issue and see if there are any problems. > I'm unsure if this over-release is related. Was this also happening without the patch? I have a suspicion the "Potential double free bug" is actually related to a different bug I was chasing today ( #107500 ), but I never previously saw the warning because of the metal-buffers issue. ... most likely nothing to see here ...

Jason Fielder added 1 commit 2023-06-19 15:23:44 +02:00

Replace custom linked list with ListBase. Fix incorrect da07958c87

counter assertion check

Jeroen Bakker commented

2023-06-19 16:02:48 +02:00

@Michael-Parkin-White-Apple I think you're right and we should revert the change of listbase. Sorry for that. ListBase isn't aware that it needs to call the destructor and could lead to other memory issues.

Michael Parkin-White commented

2023-06-19 16:17:35 +02:00

First-time contributor

@Michael-Parkin-White-Apple I think you're right and we should revert the change of listbase. Sorry for that. ListBase isn't aware that it needs to call the destructor and could lead to other memory issues.

Ahh right, so this may just free the root ptr but not the destructor?

I think we can keep the list base wrapper for the core list, but perhaps just revert to the manual iteration for release? It also looks like we can use BLI_remlink instead of BLI_freelinkN to just disconnect a link in the node, then release the memory separately.

Apologies, still familiarising with this API.

> @Michael-Parkin-White-Apple I think you're right and we should revert the change of listbase. Sorry for that. ListBase isn't aware that it needs to call the destructor and could lead to other memory issues. Ahh right, so this may just free the root ptr but not the destructor? I think we can keep the list base wrapper for the core list, but perhaps just revert to the manual iteration for release? It also looks like we can use `BLI_remlink` instead of `BLI_freelinkN` to just disconnect a link in the node, then release the memory separately. Apologies, still familiarising with this API.

Jeroen Bakker commented

2023-06-19 16:29:00 +02:00

Best not use the list base api at all. That would only confuse other developers. Just add a note why listbase isn’t used.

Jason Fielder added 2 commits 2023-06-19 16:34:19 +02:00

Revert "Replace custom linked list with ListBase. Fix incorrect" 99fafd2067