Copy-on-write data blocks could be referenced from python but were not properly managing python reference counting.
This would leak memory for any evaluated data-blocks accessed by Python.
Details
Details
Diff Detail
Diff Detail
- Repository
- rB Blender
- Branch
- T (branched from master)
- Build Status
Buildable 17792 Build 17792: arc lint + arc unit
Event Timeline
Comment Actions
Changes in the deg_expand_copy_on_write_datablock should not be needed after D12852: the function is supposed to be always used on an iD in a non-expanded state, and the py_instance will be taken care of via the Backup. I suggest verify the D12852, land it first (if verification passes), and simplify this patch.
| source/blender/depsgraph/intern/eval/deg_eval_copy_on_write.cc | ||
|---|---|---|
| 1073 | That's a bit elaborate way of freeing an ID, and is always made me sad. Is there an easier way of completely freeing ID (and not the memory) than doing 3 calls? | |
| source/blender/depsgraph/intern/eval/deg_eval_runtime_backup.cc | ||
| 57 | This is a good catch btw! | |
Comment Actions
| source/blender/depsgraph/intern/eval/deg_eval_copy_on_write.cc | ||
|---|---|---|
| 1073 | There are enough places in the code where freeing data and the python reference can't be paired up. Although it might be worth making these cases an exception, eg: BKE_libblock_free_data_ex(id, do_id_user, do_id_py_instance) Making BKE_libblock_free_data free both by default. Can be handled as a separate refactor. | |
Comment Actions
I would expect that changes in deg_expand_copy_on_write_datablock would not be needed with D12852: the deg_expand_copy_on_write_datablock is used on either non-yet-expanded or on a freed datablock. There should be no python reference at that point.
Although, need to be careful as the deg_free_copy_on_write_datablock does not guarantee the ID is fully zeroed.
Comment Actions
Ah, right, if it's not expanded it will have no Python reference, updated P2495 & add assert to ensure the Python reference is NULL.