Skip to content

feat(core, kernel): use a lock-free container in the object provider - #136

Open
lolito93 wants to merge 1 commit into
SEN-1749-local-shell-behaves-differently-to-external-shell-when-using-commands-with-optionalsfrom
SEN-1759-reduce-memory-corruption-errors-in-object-provider-before-kernel-refactoring
Open

feat(core, kernel): use a lock-free container in the object provider#136
lolito93 wants to merge 1 commit into
SEN-1749-local-shell-behaves-differently-to-external-shell-when-using-commands-with-optionalsfrom
SEN-1759-reduce-memory-corruption-errors-in-object-provider-before-kernel-refactoring

Conversation

@lolito93

Copy link
Copy Markdown
Collaborator

Resolves SEN-1759

@EnriqueParodi EnriqueParodi left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Manuel, can you please relay my feedback to the author?

The pimpl keeps xenium out of the public header, and the move operations are defaulted out of line with the destructor guarding the moved-from state. Both right.

  1. xenium's utils.hpp defines getticks for sparc, x86_64 and _M_AMD64 and #errors otherwise. The error is at file scope, so it fires on include, and utils.hpp arrives through marked_ptr.hpp.

    Nothing here uses the code that needs it. getticks only feeds utils::random(), which is called only from the two kfifo queues, and this PR uses harris_michael_list_based_set.

    The missing case can be added with a PATCH_COMMAND on the FetchContent, and is worth sending upstream since the library is missing it for all of aarch64:

    #elif defined(__aarch64__)
      static inline std::uint64_t getticks(void) {
          std::uint64_t ret;
          __asm__ volatile("mrs %0, cntvct_el0" : "=r"(ret));
          return ret;
      }
    

    We would rather patch it than turn off the arm job, which is our only non-x86 coverage.

  2. FetchContent_Declare pulls the tarball from GitHub with no URL_HASH. Everything else comes through conan and is pinned by conan.lock, which has a CI job checking for drift. This sits outside both, and a dependency we plan to remove is the one most likely to be forgotten.

No checks have run on this PR yet.

Thanks!

@EnriqueParodi
EnriqueParodi force-pushed the SEN-1749-local-shell-behaves-differently-to-external-shell-when-using-commands-with-optionals branch from 49b338d to 6155e76 Compare August 27, 2026 10:33
@EnriqueParodi
EnriqueParodi force-pushed the SEN-1759-reduce-memory-corruption-errors-in-object-provider-before-kernel-refactoring branch from 6334db7 to 13d6dd0 Compare August 27, 2026 10:33
@EnriqueParodi
EnriqueParodi marked this pull request as draft August 27, 2026 13:46
@EnriqueParodi
EnriqueParodi marked this pull request as ready for review August 27, 2026 13:46
@EnriqueParodi
EnriqueParodi marked this pull request as draft August 27, 2026 14:26
@EnriqueParodi
EnriqueParodi marked this pull request as ready for review August 27, 2026 14:26
@EnriqueParodi EnriqueParodi changed the title feat(core, kernel): reduce memory corruption errors under stress using lock-free container from xenium feat(core, kernel): use a lock-free container in the object provider Aug 27, 2026
@EnriqueParodi
EnriqueParodi marked this pull request as draft August 27, 2026 15:14
@EnriqueParodi
EnriqueParodi marked this pull request as ready for review August 27, 2026 15:14
@luisgutierrezpereda
luisgutierrezpereda force-pushed the SEN-1749-local-shell-behaves-differently-to-external-shell-when-using-commands-with-optionals branch 2 times, most recently from abc8fcc to 91ce2b4 Compare September 7, 2026 13:28
@luisgutierrezpereda

Copy link
Copy Markdown
Collaborator

Thanks for the review, @EnriqueParodi:

This is the respose:

  1. You already applied this changes
  2. Since xenium is not available on ConanCenter, adding it via FetchContent is the most straightforward approach for now. The URL_HASH should prevent any tarball drift or security risks mentioned in the comment. I'd prefer to avoid maintaining a custom, manual Conan package recipe for this single dependency, especially since we plan to remove it in the future anyway. The build compiles and works successfully with this configuration

@luisgutierrezpereda
luisgutierrezpereda force-pushed the SEN-1759-reduce-memory-corruption-errors-in-object-provider-before-kernel-refactoring branch from 13d6dd0 to 59cd9ce Compare September 7, 2026 15:00
@luisgutierrezpereda
luisgutierrezpereda changed the base branch from SEN-1749-local-shell-behaves-differently-to-external-shell-when-using-commands-with-optionals to main September 7, 2026 15:01

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Clang-Tidy found issue(s) with the introduced code (1/1)

Comment thread libs/kernel/src/runner.cpp Outdated
// std
#include <algorithm>
#include <chrono>
#include <cstddef>

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

misc-include-cleaner
included header cstddef is not used directly

Suggested change
#include <cstddef>

Comment thread libs/kernel/src/runner.cpp Outdated
// scheduling loop; turning the jump into a nested loop is a change to
// the timing path, not a lint fix, so the check is waived here.
goto doSleep; // NOLINT(cppcoreguidelines-avoid-goto,hicpp-avoid-goto)
goto doSleep; // NOLINT(hicpp-avoid-goto)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cppcoreguidelines-avoid-goto
avoid using goto for flow control

@luisgutierrezpereda
luisgutierrezpereda force-pushed the SEN-1759-reduce-memory-corruption-errors-in-object-provider-before-kernel-refactoring branch from 59cd9ce to 2fe9f3e Compare September 8, 2026 14:06

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Clang-Tidy found issue(s) with the introduced code (1/1)

Comment thread libs/kernel/src/runner.cpp Outdated
// scheduling loop; turning the jump into a nested loop is a change to
// the timing path, not a lint fix, so the check is waived here.
goto doSleep; // NOLINT(cppcoreguidelines-avoid-goto,hicpp-avoid-goto)
goto doSleep; // NOLINT(cppcoreguidelines-avoid-goto)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hicpp-avoid-goto
avoid using goto for flow control

@luisgutierrezpereda
luisgutierrezpereda force-pushed the SEN-1759-reduce-memory-corruption-errors-in-object-provider-before-kernel-refactoring branch 2 times, most recently from 72df445 to 0a68785 Compare September 9, 2026 14:08
@luisgutierrezpereda
luisgutierrezpereda changed the base branch from main to SEN-1749-local-shell-behaves-differently-to-external-shell-when-using-commands-with-optionals September 9, 2026 14:08
@luisgutierrezpereda
luisgutierrezpereda force-pushed the SEN-1759-reduce-memory-corruption-errors-in-object-provider-before-kernel-refactoring branch 2 times, most recently from 2c1db97 to 3f75bc8 Compare September 9, 2026 14:26
@luisgutierrezpereda
luisgutierrezpereda force-pushed the SEN-1749-local-shell-behaves-differently-to-external-shell-when-using-commands-with-optionals branch 2 times, most recently from 6487e46 to 5de3ba3 Compare September 10, 2026 08:27
Replaces the object provider's container with xenium's lock-free one to
reduce memory corruption under stress, ahead of the kernel refactoring.

Resolves SEN-1759
@luisgutierrezpereda
luisgutierrezpereda force-pushed the SEN-1759-reduce-memory-corruption-errors-in-object-provider-before-kernel-refactoring branch from 3f75bc8 to b1650b6 Compare September 10, 2026 08:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants