refactor(userspace/libsinsp)!: reduce usage of raw pointers

[jasondellaluce]

What type of PR is this?

/kind bug

/kind cleanup

/kind design

Any specific area of the project related to this PR?

/area libsinsp

Does this PR require a change in the driver versions?

What this PR does / why we need it:

This has been discussed for a while, and also during one of the past few maintainers call. Basically, the usage of raw pointers is the root of many debugging nightmares and memory-related issues both during the development cycles and at release time.

This PR hunts for all the evident places where a raw pointer could be substituted by a unique_ptr. This has been "optimized" to be as low impact as possible in terms of LOC, so I didn't spend too much time refactoring some legacy code parts like chisels but put extra attention to the core codebase. The most important components involved in the changes are the ones of thread management, filterchecks, filters, formatters, and their factory classes/methods. Overall, most of raw pointers are now gone.

Which issue(s) this PR fixes:

Special notes for your reviewer:

With this, most of our pointers will be safely managed as unique or shared. However, my opinion is that we may have put too many pointed as shared (with ref count) whereas they could be simpler unique_ptr in many cases. Will look into this if/after this one goes through.

Does this PR introduce a user-facing change?:

refactor(userspace/libsinsp)!: reduce usage of raw pointers

[incertum]

/approve

Yes this is great, plus some const correction as well, thank you!

[poiana]

LGTM label has been added.

Git tree hash: 9b424b4f2652de9ed90c38a38eadac61e8edb8cc

[incertum]

/milestone 0.15.0

[incertum]

However, my opinion is that we may have put too many pointed as shared (with ref count) whereas they could be simpler unique_ptr in many cases. Will look into this if/after this one goes through.

Agreed to separate this and check later.

This has been discussed for a while, and also during one of the past few maintainers call. Basically, the usage of raw pointers is the root of many debugging nightmares and memory-related issues both during the development cycles and at release time.

That's right!

[FedeDP]

Left a couple of questions; aside them, PR LGTM! Great job @jasondellaluce !
Also, since some public methods changed interface, i'd add the ! breaking change signal :D

[FedeDP]

/hold

[Molter73]

Side note, I do like we are trying to improve our handling of memory and there's active work on it, I just fear making these changes assuming things like std::unique_ptr will not have a meaningful performance impact on the code as a whole could turn us from discussing "we have memory errors" into "we have performance issues".

TL;DR we need better performance testing alongside these sorts of changes IMO.

[jasondellaluce]

@Molter73 I appreciate raising concerns about performance, definitely onboard with double checking. My thesis is that the adoption of unique_ptrs in this contribution will add no visible cost to our critical paths.

I went on and ran some benchmarks of our internal products to see if this change could negatively affect performance. This led to non-visibile performance regression compared to the current mainline.

To be extra-sure and transparent, I went on and played around with Compiler Explorer (https://godbolt.org/) to see how this gets compiled in practice. The compiler is a gcc 13.2 x86_64 with -O1 option (we compile with -O3 in release mode, so expect smarter compiler tricks being applied).

Raw ptr example	Unique ptr example
#include #include int* new_int() { return new int{5}; } int main() { auto value = new_int(); std::cout << *value << std::endl; delete value; return 0; }	#include #include std::unique_ptr new_int() { return std::make_unique(5); } int main() { auto value = new_int(); std::cout << *value << std::endl; return 0; }
new_int(): sub rsp, 8 mov edi, 4 call operator new(unsigned long) mov DWORD PTR [rax], 5 add rsp, 8 ret main: push rbx call new_int() mov rbx, rax mov esi, DWORD PTR [rax] mov edi, OFFSET FLAT:_ZSt4cout call ... operator<<(int) mov rdi, rax call std::basic_ostream<char ... test rbx, rbx je .L4 mov esi, 4 mov rdi, rbx call operator delete(void* ... .L4: mov eax, 0 pop rbx ret	new_int(): push rbx mov rbx, rdi mov edi, 4 call operator new(unsigned long) mov DWORD PTR [rax], 5 mov QWORD PTR [rbx], rax mov rax, rbx pop rbx ret main: push rbx sub rsp, 16 lea rdi, [rsp+8] call new_int() mov rax, QWORD PTR [rsp+8] mov esi, DWORD PTR [rax] mov edi, OFFSET FLAT:_ZSt4cout call ... operator<<(int) mov rdi, rax call std::basic_ostream<char ... jmp .L15 mov rbx, rax lea rdi, [rsp+8] call ... ~unique_ptr() mov rdi, rbx call _Unwind_Resume .L15: lea rdi, [rsp+8] call ... ~unique_ptr() mov eax, 0 add rsp, 16 pop rbx ret

Some minor differences are visibile, but as you can see it only comes down to few POP and MOV instructions due to the C++ ABI, and everything else is all inlined and completely equivalent to the raw pointer case. Note, the block added on the bottom is there only for exception handling code (makes sure the ptr is deleted in case of exception), which is far more optimized than catching the exception by hand (which we also don't do in most of the codebase).

To me, this seems like negligible and acceptable impact.

[jasondellaluce]

@Molter73 @FedeDP I tried to address all the suggestions and concerns. Please let me know if you see anything additional as a blocker.

[FedeDP]

/approve

[poiana]

LGTM label has been added.

Git tree hash: 80da7b370bea21dec71bf8edf9a899d4c064219f

[Molter73]

/lgtm

[poiana]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: FedeDP, incertum, jasondellaluce, Molter73

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [FedeDP,Molter73,incertum,jasondellaluce]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[jasondellaluce]

/unhold