Skip to content

Make dn_simdhash OOM safe #117059

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Make dn_simdhash OOM safe #117059

wants to merge 2 commits into from

Conversation

kg
Copy link
Member

@kg kg commented Jun 26, 2025

Like its predecessor ghashtable, dn_simdhash was not written to handle malloc failures. Thanks to @davidwrighton for reminding me that this needed to be fixed.

Note that the 'ght compatible' version of simdhash asserts on malloc failure, since there's no way to retrofit it to allow the caller to do anything else - all the existing call sites in mono would become wrong if we did that.

The 'new' operation now returns NULL on malloc failure.
The add operations now return an enum where a positive value indicates success, a negative value indicates OOM, and a 0 indicates failure.
The ensure capacity operation now returns a success bool, where 0 means malloc failed and 1 means that the hash is big enough to hold the capacity you requested.

This PR also adds the new dn_simdhash_ght_try_insert API which allows you to sense OOM when using the ght compatible variant.

@Copilot Copilot AI review requested due to automatic review settings June 26, 2025 16:42
@kg kg requested review from BrzVlad and janvorli as code owners June 26, 2025 16:42
@github-actions github-actions bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jun 26, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates dn_simdhash to handle allocation failures by returning error codes rather than asserting or leaking memory. Key changes include:

  • Introducing dn_simdhash_add_result to report add operation outcomes including OOM.
  • Changing ensure_capacity_internal and ensure_capacity to return a success flag.
  • Propagating and asserting on OOM in all call sites (specializations, ght-compatible, and the CoreCLR stackmap).

Reviewed Changes

Copilot reviewed 7 out of 8 changed files in this pull request and generated no comments.

Show a summary per file
File Description
run-benchmark.ps1 Added dn-simdhash-ght-compatible.c to the compile command
src/native/containers/dn-simdhash.h Added dn_simdhash_add_result enum and updated capacity APIs
src/native/containers/dn-simdhash.c Checked malloc return values and propagated OOM in constructors and capacity logic
src/native/containers/dn-simdhash-specialization.h Changed TRY_ADD functions to return dn_simdhash_add_result
src/native/containers/dn-simdhash-specialization-declarations.h Updated declarations to use dn_simdhash_add_result
src/native/containers/dn-simdhash-ght-compatible.c Added allocation checks and assertions for ght-compatible API
src/coreclr/interpreter/stackmap.cpp Assert on creation and add return codes in the shared stack map
Comments suppressed due to low confidence (3)

src/native/containers/dn-simdhash.h:56

  • Add unit tests that simulate allocation failures to verify that DN_SIMDHASH_OUT_OF_MEMORY is correctly returned from both add and ensure_capacity operations.
typedef enum dn_simdhash_add_result {

src/native/containers/dn-simdhash.h:56

  • [nitpick] Include comments explaining each dn_simdhash_add_result enumerator value (e.g., DN_SIMDHASH_ADD_FAILED vs DN_SIMDHASH_OUT_OF_MEMORY) to clarify their distinct semantics for API consumers.
typedef enum dn_simdhash_add_result {

src/native/containers/simdhash-benchmark/run-benchmark.ps1:1

  • The added line has an unexpected leading '1 ' before the '+'. It should start with '+cl ...' without the extra '1 ' to correctly include the new source file in the compile command.
cl /GS- /O2 /std:c17 ./*.c ../dn-simdhash-u32-ptr.c ../dn-simdhash.c ../dn-vector.c ../dn-simdhash-string-ptr.c ../dn-simdhash-ght-compatible.c /DNO_CONFIG_H /DSIZEOF_VOID_P=8 /DNDEBUG /Fe:all-measurements.exe

@kg
Copy link
Member Author

kg commented Jun 26, 2025

Updated to add a (speculative) dn_simdhash_ght_try_insert API that lets you explicitly specify which insert mode you want and sense whether it OOMed.

@kg kg force-pushed the simdhash-oom-safety branch from 8623ded to 3746241 Compare June 27, 2025 17:55
@am11 am11 added area-Interop-coreclr and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Jun 27, 2025
Copy link
Contributor

Tagging subscribers to this area: @dotnet/interop-contrib
See info in area-owners.md if you want to be subscribed.

kg added 2 commits June 28, 2025 13:43
… they can signal OOM and whether an overwrite happened

Make simdhash handle malloc failures during new and resize operations

Fix some zeros where NULL would do

Move side effect out of assert

Explicitly NOMEM if the simdhash add fails due to an out of memory condition

Add dn_simdhash_ght_try_insert

Cleanup some erroneous uses of assert() instead of dn_simdhash_assert()
Address PR feedback

Cleanup simdhash failure handling

Cleanup

Fix CI warnings
@kg kg force-pushed the simdhash-oom-safety branch from 0ddf0ad to 57a58d6 Compare June 28, 2025 21:00
@kg
Copy link
Member Author

kg commented Jun 28, 2025

Since we had a conflict in the compiler code I took the opportunity to refactor a bit, there's a failures.h where NOMEM lives now (so files other than compiler can use it) and a simdhash.h that contains the ptr_ptr_holder and result check helpers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants