Fix: Cycles memory leak in HIP-RT #126788

Sergey Sharybin · 2024-08-26T15:26:06+02:00

Sergey Sharybin commented

2024-08-26 15:26:06 +02:00

Some of the device memory objects had their host_pointer overwritten
with another CPU-side buffer after allocation. This leads to a leak of
host memory allocated by the device_memory.

There are few remaining places where the host_pointer is assigned and
those seems to be fine because the memory was not yet allocated with
a alloc() call.

While the approach in this change is not very ideal, it is small and
potentially could be ported to the LTS tracks. More ideal solution
would be to utilize device_vector::give_data().

Some of the device memory objects had their host_pointer overwritten with another CPU-side buffer after allocation. This leads to a leak of host memory allocated by the device_memory. There are few remaining places where the host_pointer is assigned and those seems to be fine because the memory was not yet allocated with a alloc() call. While the approach in this change is not very ideal, it is small and potentially could be ported to the LTS tracks. More ideal solution would be to utilize device_vector::give_data().

Sergey Sharybin added 1 commit 2024-08-26 15:26:14 +02:00

Fix: Cycles memory leak in HIP-RT

buildbot/vexp-code-patch-lint Build done.

Details

buildbot/vexp-code-patch-linux-x86_64 Build done.

Details

buildbot/vexp-code-patch-darwin-x86_64 Build done.

Details

buildbot/vexp-code-patch-windows-amd64 Build done.

Details

buildbot/vexp-code-patch-darwin-arm64 Build done.

Details

buildbot/vexp-code-patch-coordinator Build done.

Details

5f689b48c2

Some of the device memory objects had their host_pointer overwritten
with another CPU-side buffer after allocation. This leads to leak of
device memory allocated by the device memory.

There are few remaining places where the host_pointer is assigned and
those seems to be fine because the memory was not yet allocated with
a alloc() call.

While the approach in this change is not very ideal, it is small and
potentially could be ported to the LTS tracks. More ideal solution
would be to utilize device_vector::give_data().

Sergey Sharybin commented

2024-08-26 15:26:56 +02:00

@blender-bot build

Sergey Sharybin requested review from Alaska 2024-08-26 15:27:07 +02:00

Sergey Sharybin requested review from Sahar A. Kashi 2024-08-26 15:27:07 +02:00

Sergey Sharybin referenced this pull request

2024-08-26 15:36:11 +02:00

Cycles: Linux Support for HIP-RT #121050

Sahar A. Kashi approved these changes 2024-08-27 04:17:23 +02:00

Alaska commented

2024-08-27 07:47:53 +02:00

It's not clear to me how to best test this PR to ensure it's working. Both of the HIP-RT memory leaks that I know of (#120702, #126749) are still present with this PR and a old version of HIP-RT (2.0.something) on Windows.

Do you have steps to reproduce the issue?

...This leads to leak of
device memory allocated by the device memory.

I believe there's a mistake here in your commit message.

It's not clear to me how to best test this PR to ensure it's working. Both of the HIP-RT memory leaks that I know of (#120702, #126749) are still present with this PR and a old version of HIP-RT (2.0.something) on Windows. Do you have steps to reproduce the issue? > ...This leads to leak of > device memory allocated by the device memory. I believe there's a mistake here in your commit message.

Sergey Sharybin commented

2024-08-27 10:20:29 +02:00

Do you have steps to reproduce the issue?

The steps are relatively easy:

Render default cube using HIP-RT.
Quit Blender.
Check the console output.

If you run blender --debug-memory you'll see a couple of non-freed memory blocks named "Cycles Aligned Alloc". More you render, or more objects you have, more leaked memory blocks you'll see.

This is something I've noticed when looking into #121050.
If you are on Linux, you'd need to apply that PR, compile HIP-RT, and only then you'd be able to reproduce the issue.

On Windows I think you can see the issue with any recent build, as we do support HIP-RT there for a while. However, I do not currently have a Windows install on the machine with AMD card, so I can not verify this.

I believe there's a mistake here in your commit message.

Indeed. Corrected the message.

> Do you have steps to reproduce the issue? The steps are relatively easy: - Render default cube using HIP-RT. - Quit Blender. - Check the console output. If you run `blender --debug-memory` you'll see a couple of non-freed memory blocks named "Cycles Aligned Alloc". More you render, or more objects you have, more leaked memory blocks you'll see. This is something I've noticed when looking into #121050. If you are on Linux, you'd need to apply that PR, compile HIP-RT, and only then you'd be able to reproduce the issue. On Windows I think you can see the issue with any recent build, as we do support HIP-RT there for a while. However, I do not currently have a Windows install on the machine with AMD card, so I can not verify this. > I believe there's a mistake here in your commit message. Indeed. Corrected the message.

Alaska commented

2024-08-27 10:29:10 +02:00

Do you have steps to reproduce the issue?

The steps are relatively easy:

Render default cube using HIP-RT.

Quit Blender.

Check the console output.

In that case, I can confirm the issue on Windows in main, and can confirm it's fixed with this PR. I'll approve the change.

> > Do you have steps to reproduce the issue? > > The steps are relatively easy: > - Render default cube using HIP-RT. > - Quit Blender. > - Check the console output. In that case, I can confirm the issue on Windows in main, and can confirm it's fixed with this PR. I'll approve the change.

Alaska approved these changes 2024-08-27 10:29:20 +02:00

Sergey Sharybin commented

2024-08-27 12:46:34 +02:00

Thanks for testing and reviews.

Sergey Sharybin merged commit 92733a9415 into main

2024-08-27 12:47:11 +02:00

Sergey Sharybin referenced this issue from a commit

2024-08-27 12:47:12 +02:00

Fix: Cycles memory leak in HIP-RT

Sergey Sharybin deleted branch cycles_hip_memory_leak

2024-08-27 12:47:14 +02:00

Alaska referenced this issue from a commit