Memory leak and design violation in curve evaluation #96498

New Issue

Sergey Sharybin · 2022-03-15T17:45:42+01:00

Sergey Sharybin commented

2022-03-15 17:45:42 +01:00

System Information
Operating system: Linux-5.16.0-4-amd64-x86_64-with-glibc2.33 64 Bits
Graphics card: Quadro RTX 6000/PCIe/SSE2 NVIDIA Corporation 4.5.0 NVIDIA 510.54

Blender Version
Broken: version: 3.2.0 Alpha, branch: Unknown, commit date: Unknown Unknown, hash: rBUnknown
Worked: Prior to D11597 c946fdb2e5

Short description of error

Rendering on playing back lib/tests/render/motion_blur/multi_step_motion_blur.blend results in memory leak.

Exact steps for others to reproduce the error

Easiest to see if LSAN is enabled, but guarded allocator also catches the issue.

Way 1: Open the file, hit F12, observe memory leak printed on exit.
Way 2: Open the file, hit playback, exit

There are two things which on a glance seems to be wrong in the new implementation (since the mentioned patch) in the BKE_displist_make_curveTypes.

The ob->data at the moment of the function is called is not "localized" to the object: if multiple objects are sharing the same curve multiple copy-on-write objects will share the same copy-on-write curve. This means that writing to cow_curve.curve_eval is "illegal". The general rule: do not write to object-data level when evaluating object. This general rule is violated without either explanation or realization that it is something bad.

The freeing of Curve::curve_eval is only happening in the curve_free_data, which means evaluating same depsgraph at different times will not free this data: cow_curve.curve_eval = nullptr;.

P.S. The comment in the Curve::CurveEval states Owned by the object's #geometry_set_eval. That's the giveaway: something owned by object level can not be stored in the object-data level.

**System Information** Operating system: Linux-5.16.0-4-amd64-x86_64-with-glibc2.33 64 Bits Graphics card: Quadro RTX 6000/PCIe/SSE2 NVIDIA Corporation 4.5.0 NVIDIA 510.54 **Blender Version** Broken: version: 3.2.0 Alpha, branch: Unknown, commit date: Unknown Unknown, hash: `rBUnknown` Worked: Prior to [D11597](https://archive.blender.org/developer/D11597) c946fdb2e5 **Short description of error** Rendering on playing back `lib/tests/render/motion_blur/multi_step_motion_blur.blend` results in memory leak. **Exact steps for others to reproduce the error** Easiest to see if LSAN is enabled, but guarded allocator also catches the issue. Way 1: Open the file, hit F12, observe memory leak printed on exit. Way 2: Open the file, hit playback, exit There are two things which on a glance seems to be wrong in the new implementation (since the mentioned patch) in the `BKE_displist_make_curveTypes`. The `ob->data` at the moment of the function is called is not "localized" to the object: if multiple objects are sharing the same curve multiple copy-on-write objects will share the same copy-on-write curve. This means that writing to `cow_curve.curve_eval` is "illegal". The general rule: do not write to object-data level when evaluating object. This general rule is violated without either explanation or realization that it is something bad. The freeing of `Curve::curve_eval` is only happening in the `curve_free_data`, which means evaluating same depsgraph at different times will not free this data: `cow_curve.curve_eval = nullptr;`. P.S. The comment in the `Curve::CurveEval` states `Owned by the object's #geometry_set_eval`. That's the giveaway: something owned by object level can not be stored in the object-data level.

Sergey Sharybin commented

2022-03-15 17:45:42 +01:00

Added subscriber: @Sergey

Hans Goudey commented

2022-03-15 18:01:27 +01:00

Changed status from 'Needs Triage' to: 'Confirmed'

Hans Goudey self-assigned this 2022-03-15 18:01:27 +01:00

Hans Goudey commented

2022-03-16 05:52:13 +01:00

Yikes, I just realized the problem is worse than I expected. Even in 3.1, a modifier on a single curve object affects all users of the original curve. I'm amazed that behavior made it so long in master.
{F12929626 size=full}

I can relatively easily fix that problem by moving the final evaluated CurveEval around to a different place, but not yet without causing other problems. Maybe I'll have better luck tomorrow.

Really though, I'm thinking that the easiest/best way to fix this might be just using the new curves object for rendering. That's already planned for the next week or two anyway.

Yikes, I just realized the problem is worse than I expected. Even in 3.1, a modifier on a single curve object affects all users of the original curve. I'm amazed that behavior made it so long in master. {[F12929626](https://archive.blender.org/developer/F12929626/image.png) size=full} I can relatively easily fix that problem by moving the final evaluated `CurveEval` around to a different place, but not yet without causing other problems. Maybe I'll have better luck tomorrow. Really though, I'm thinking that the easiest/best way to fix this might be just using the new curves object for rendering. That's already planned for the next week or two anyway.

Sergey Sharybin commented

2022-03-16 09:56:18 +01:00

I'm thinking that the easiest/best way to fix this might be just using the new curves object for rendering

Will this allow us to get rid of CurveEval from the Curve datablock?
Other thought to keep in mind: is this issue something we want to be fixed in possible upcoming corrective release for 3.1?

Other than that, focusing on the proper solution is surely better idea than trying to find a quicker and less ideal fix.

> I'm thinking that the easiest/best way to fix this might be just using the new curves object for rendering Will this allow us to get rid of `CurveEval` from the `Curve` datablock? Other thought to keep in mind: is this issue something we want to be fixed in possible upcoming corrective release for 3.1? Other than that, focusing on the proper solution is surely better idea than trying to find a quicker and less ideal fix.

Hans Goudey commented

2022-03-21 13:49:44 +01:00

In #96498#1323899, @Sergey wrote:
Will this allow us to get rid of CurveEval from the Curve datablock?

Yes, it would, since Curve wouldn't be used for rendering at all. We would just need a way to draw the Curve edit mode overlays from the Curves data type, which sounds a bit weird, but maybe still better than alternatives.

Other thought to keep in mind: is this issue something we want to be fixed in possible upcoming corrective release for 3.1?

Hmm, I don't really think so, it's a bad bug but also a tenuous situation, not an area I'd change in a corrective release.

> In #96498#1323899, @Sergey wrote: > Will this allow us to get rid of `CurveEval` from the `Curve` datablock? Yes, it would, since `Curve` wouldn't be used for rendering at all. We would just need a way to draw the `Curve` edit mode overlays from the `Curves` data type, which sounds a bit weird, but maybe still better than alternatives. > Other thought to keep in mind: is this issue something we want to be fixed in possible upcoming corrective release for 3.1? Hmm, I don't really think so, it's a bad bug but also a tenuous situation, not an area I'd change in a corrective release.

blender-admin commented

2022-04-22 17:26:08 +02:00

This issue was referenced by a7c65ef4cb

This issue was referenced by a7c65ef4cbbd71972170cc8406a85b2332d304f2

Hans Goudey commented

2022-04-22 17:27:36 +02:00

Changed status from 'Confirmed' to: 'Resolved'

Hans Goudey closed this issue

2022-04-22 17:27:36 +02:00

Sign in to join this conversation.

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Memory leak and design violation in curve evaluation #96498