Maniphest T96498

Memory leak and design violation in curve evaluation
Closed, ResolvedBUG

Assigned To
Hans Goudey (HooglyBoogly)
Authored By
Sergey Sharybin (sergey)
Mar 15 2022, 5:45 PM
Tags
  • BF Blender
  • Nodes & Physics
  • Geometry Nodes
Subscribers
Sergey Sharybin (sergey)

Description

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 rBc946fdb2e5f

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.

Revisions and Commits

rB Blender
D14561

Related Objects

Mentioned In
rBb84255f590c0: Curves: Port legacy curve viewport drawing to the new data-block
D14551: Curves: Port legacy curve viewport drawing to the new data-block
Mentioned Here
rBc946fdb2e5fc: Calm Warning: Unused Variable
D11597: Geometry Nodes: Support modifier on curve objects

Event Timeline

Sergey Sharybin (sergey) created this task.Mar 15 2022, 5:45 PM
Hans Goudey (HooglyBoogly) changed the task status from Needs Triage to Confirmed.Mar 15 2022, 6:01 PM
Hans Goudey (HooglyBoogly) claimed this task.
Hans Goudey (HooglyBoogly) changed the subtype of this task from "Report" to "Bug".
Hans Goudey (HooglyBoogly) added a project: Nodes & Physics.
Hans Goudey (HooglyBoogly) added a comment.Mar 16 2022, 5:52 AM

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.

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 (sergey) added a comment.Mar 16 2022, 9:56 AM

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 (HooglyBoogly) added a comment.Mar 21 2022, 1:49 PM
In T96498#1323899, @Sergey Sharybin (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.

Hans Goudey (HooglyBoogly) triaged this task as High priority.Mar 21 2022, 1:50 PM
Hans Goudey (HooglyBoogly) added a project: Geometry Nodes.
Hans Goudey (HooglyBoogly) mentioned this in rBb84255f590c0: Curves: Port legacy curve viewport drawing to the new data-block.Apr 5 2022, 6:42 PM
Hans Goudey (HooglyBoogly) moved this task from Backlog/Bugs to In Progress on the Geometry Nodes board.Apr 6 2022, 12:10 AM
Hans Goudey (HooglyBoogly) moved this task from In Progress to Waiting for Feedback/Review on the Geometry Nodes board.Apr 6 2022, 2:15 AM
Hans Goudey (HooglyBoogly) moved this task from Waiting for Feedback/Review to In Progress on the Geometry Nodes board.Apr 20 2022, 5:52 AM
Hans Goudey (HooglyBoogly) closed this task as Resolved by committing rBa7c65ef4cbbd: Fix T96498: Modifiers affect multiple curve objects.Apr 22 2022, 5:27 PM
Hans Goudey (HooglyBoogly) added a commit: rBa7c65ef4cbbd: Fix T96498: Modifiers affect multiple curve objects.