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

Fix BEAM crash from from port/NIF thread #8209

Merged
merged 1 commit into from
Feb 18, 2025

Conversation

dotsimon
Copy link
Contributor

@dotsimon dotsimon commented Feb 29, 2024

This patch prevents non-scheduler threads from crashing the emulator.
Fixes #8208

Copy link
Contributor

github-actions bot commented Feb 29, 2024

CT Test Results

    3 files    141 suites   49m 25s ⏱️
1 602 tests 1 553 ✅ 49 💤 0 ❌
2 311 runs  2 242 ✅ 69 💤 0 ❌

Results for commit 698fe88.

♻️ This comment has been updated with latest results.

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

// Erlang/OTP Github Action Bot

@dotsimon dotsimon force-pushed the ext_large_maps_crash branch 3 times, most recently from 6bdb8fc to d2e8b15 Compare March 1, 2024 07:08
@rickard-green rickard-green added the team:VM Assigned to OTP team VM label Mar 4, 2024
@sverker sverker force-pushed the ext_large_maps_crash branch from d2e8b15 to 7e66483 Compare March 4, 2024 13:51
@sverker
Copy link
Contributor

sverker commented Mar 4, 2024

Rebased onto OTP-23.3.4 + 184634a which is mergeable to all newer release branches.

Also tweaked solution with a global atomic to get some "randomness".

@dotsimon
Copy link
Contributor Author

dotsimon commented Mar 5, 2024

The sample I attached to #8208 (and now it is actually attached!) includes a rudimentary benchmark. There's a lot of variance in the results but they seem to consistently be a few % slower with Sverker's tweak. And in that simple test there is also no contention for the new atomic which I imagine would further degrade performance.
So how important is that "randomness"?

@sverker
Copy link
Contributor

sverker commented Mar 5, 2024

What if you change erts_sched_local_random_nosched_state to an Uint and just do

rand_state = erts_sched_local_random_nosched_state++;

@dotsimon
Copy link
Contributor Author

dotsimon commented Mar 5, 2024

If something like this was what you had in mind then it was about the same.

@jhogberg
Copy link
Contributor

jhogberg commented Mar 6, 2024

Given that the random number is used to pick a random pivot element in quicksort, performance is expected to vary as a result of this change, and the consistently worse performance might just be bad luck (or rather, very good luck with the original implementation always returning a good constant).

@sverker sverker added fix testing currently being tested, tag is used by OTP internal CI labels Feb 12, 2025
@sverker sverker force-pushed the ext_large_maps_crash branch from 7e66483 to 698fe88 Compare February 17, 2025 12:50
@sverker sverker requested review from jhogberg and sverker February 17, 2025 12:52
sverker
sverker previously approved these changes Feb 17, 2025
Copy link
Contributor

@sverker sverker left a comment

Choose a reason for hiding this comment

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

@dotsimon Forgot about this PR. Now rebased into one commit and push to your repo.

@sverker sverker changed the base branch from master to maint February 17, 2025 13:03
@sverker sverker dismissed their stale review February 17, 2025 13:03

The base branch was changed.

@sverker sverker merged commit 3e23b4f into erlang:maint Feb 18, 2025
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix team:VM Assigned to OTP team VM testing currently being tested, tag is used by OTP internal CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SEGV crash with externally encoded large maps from port/NIF thread
4 participants