RNA: return PointerRNA from rna create functions #111976

Jacques Lucke · 2023-09-05T14:31:22+02:00

Jacques Lucke commented

2023-09-05 14:31:22 +02:00

There are a couple of functions that create rna pointers. For example RNA_main_pointer_create and RNA_pointer_create. Currently, those take an output parameter r_ptr as last argument. This patch changes it so that the functions actually return a PointerRNA instead of using the output parameters.

This has a few benefits:

Output parameters should only be used when there is an actual benefit. Otherwise, one should default to returning the value.
It's simpler to use the API in the large majority of cases (note that this patch reduces the number of lines of code).
It allows the PointerRNA to be const on the call-site, if that is desired.

No performance regression has been measured in production files. If one of these functions happened to be called in a hot loop where there is a regression, the solution should be to use an inline function there which allows the compiler to optimize it even better.

Currently, this patch only changes RNA_main_pointer_create and RNA_id_pointer_create. I'll update the other ones as well, if people are generally fine with this change.

There are a couple of functions that create rna pointers. For example `RNA_main_pointer_create` and `RNA_pointer_create`. Currently, those take an output parameter `r_ptr` as last argument. This patch changes it so that the functions actually return a` PointerRNA` instead of using the output parameters. This has a few benefits: * Output parameters should only be used when there is an actual benefit. Otherwise, one should default to returning the value. * It's simpler to use the API in the large majority of cases (note that this patch reduces the number of lines of code). * It allows the `PointerRNA` to be const on the call-site, if that is desired. No performance regression has been measured in production files. If one of these functions happened to be called in a hot loop where there is a regression, the solution should be to use an inline function there which allows the compiler to optimize it even better. Currently, this patch only changes `RNA_main_pointer_create` and `RNA_id_pointer_create`. I'll update the other ones as well, if people are generally fine with this change.

❤️ 1 🎉 1

Jacques Lucke added 2 commits 2023-09-05 14:31:37 +02:00

2e0d7cf5e7 update RNA_main_pointer_create

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

d39ac7dcec update RNA_id_pointer_create

Jacques Lucke commented

2023-09-05 14:32:20 +02:00

@blender-bot build

Jacques Lucke requested review from Brecht Van Lommel 2023-09-05 14:32:41 +02:00

Jacques Lucke requested review from Campbell Barton 2023-09-05 14:32:41 +02:00

Jacques Lucke requested review from Sergey Sharybin 2023-09-05 14:32:41 +02:00

Jacques Lucke requested review from Bastien Montagne 2023-09-05 14:32:41 +02:00

Jacques Lucke requested review from Sybren A. Stüvel 2023-09-05 14:32:50 +02:00

Bastien Montagne reviewed 2023-09-05 14:37:19 +02:00

Bastien Montagne left a comment

I would guess the reason behind current code was to avoid copying around PointerRNA data?

No idea if this had ever any significant impact on performances, or if is still relevant nowadays?

If there is no significant impact, change indeed totally makes sense to me.

I would guess the reason behind current code was to avoid copying around PointerRNA data? No idea if this had ever any significant impact on performances, or if is still relevant nowadays? If there is no significant impact, change indeed totally makes sense to me.

Jacques Lucke commented

2023-09-05 14:43:04 +02:00

I would be very surprised if this has any performance impact in practice. If this had a measurable performance impact, then I think the solution would be to inline these small functions. I'd expect the function call overhead here to be larger than actually changing the pointer. Especially in the common case where the PointerRNA lives on the stack.

I would be very surprised if this has any performance impact in practice. If this had a measurable performance impact, then I think the solution would be to inline these small functions. I'd expect the function call overhead here to be larger than actually changing the pointer. Especially in the common case where the `PointerRNA` lives on the stack.

Campbell Barton approved these changes 2023-09-05 14:47:29 +02:00

Sybren A. Stüvel approved these changes 2023-09-05 14:49:47 +02:00

Sybren A. Stüvel left a comment

LGTM!

Campbell Barton commented

2023-09-05 14:51:08 +02:00

GCC isn't able to reliably optimize return values from functions, causing them to be less efficient than passing a pointer at times. On my system with GCC 13.2.1 the resulting binary is ~3kb larger - while I don't think it's a blocker, noting this has some minor down-sides.

Brecht Van Lommel approved these changes 2023-09-05 16:32:19 +02:00

Bastien Montagne approved these changes 2023-09-05 16:52:27 +02:00

Sergey Sharybin approved these changes 2023-09-05 17:20:23 +02:00

Jacques Lucke added 4 commits 2023-09-06 00:14:09 +02:00

e1dc91b4f8 Merge branch 'main' into return-rna-pointer

568eee821c update RNA_blender_rna_pointer_create

998316cca2 update RNA_pointer_recast

e46338398d update RNA_pointer_create

Jacques Lucke added 1 commit 2023-09-06 00:15:11 +02:00