Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert "Revert "Feature/remove needless id lookup"" #460

Merged

Conversation

mgovers
Copy link
Member

@mgovers mgovers commented Jan 4, 2024

Re-apply #459

@mgovers mgovers added the bug Something isn't working label Jan 4, 2024
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
@mgovers mgovers force-pushed the revert-459-revert-456-feature/remove-needless-id-lookup branch from 9c0dca1 to c8b6b74 Compare January 4, 2024 14:55
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
@TonyXiang8787
Copy link
Member

@mgovers can you elaborate the problem here?

@mgovers
Copy link
Member Author

mgovers commented Jan 5, 2024

@mgovers can you elaborate the problem here?

I am still digging further into it, but the SIGBUS error suggests that there is either an alignment issue. (see also e.g. https://www.geeksforgeeks.org/segmentation-fault-sigsegv-vs-bus-error-sigbus/ ).

Steffan mentioned that on virtual devices (e.g. a container) even the stack memory is not necessarily contiguous. Especially when dealing with multi-threading, virtualization may result in unexpected behaviour when accessing stack memory of another thread (due to potentially different offsets). Workarounds involve either allocating memory on the heap (which has built-in support for non-contiguous memory access), or giving each thread its own stack.

From this information, I deduced that issue likely arose in the original implementation because of a copy of a reference to stack memory initialized in another thread. The new implementation does not have that issue.

This PR is still in draft because investigation whether this change will solve the issue is very much in progress. I am not 100% sure that what I was told about virtualization is correct in the first place, let alone that this part of the change was the issue, and that my new change solves that.

Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
…-feature/remove-needless-id-lookup

Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
@mgovers
Copy link
Member Author

mgovers commented Jan 9, 2024

@mgovers can you elaborate the problem here?

I am still digging further into it, but the SIGBUS error suggests that there is either an alignment issue. (see also e.g. https://www.geeksforgeeks.org/segmentation-fault-sigsegv-vs-bus-error-sigbus/ ).

Steffan mentioned that on virtual devices (e.g. a container) even the stack memory is not necessarily contiguous. Especially when dealing with multi-threading, virtualization may result in unexpected behaviour when accessing stack memory of another thread (due to potentially different offsets). Workarounds involve either allocating memory on the heap (which has built-in support for non-contiguous memory access), or giving each thread its own stack.

From this information, I deduced that issue likely arose in the original implementation because of a copy of a reference to stack memory initialized in another thread. The new implementation does not have that issue.

This PR is still in draft because investigation whether this change will solve the issue is very much in progress. I am not 100% sure that what I was told about virtualization is correct in the first place, let alone that this part of the change was the issue, and that my new change solves that.

The SIGBUS and SIGSEGV issues were not caused by this PR but by #461 and a mitigation was made in #462 . A sustainable solution will be made independently from this PR

Copy link

sonarcloud bot commented Jan 9, 2024

Quality Gate Passed Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

2 New issues
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@mgovers mgovers marked this pull request as ready for review January 9, 2024 14:55
@mgovers mgovers merged commit fc8a716 into main Jan 9, 2024
26 checks passed
@mgovers mgovers deleted the revert-459-revert-456-feature/remove-needless-id-lookup branch January 9, 2024 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants