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

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

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    143 suites   55m 27s ⏱️
1 583 tests 1 533 ✅ 50 💤 0 ❌
2 682 runs  2 610 ✅ 72 💤 0 ❌

Results for commit 7e66483.

♻️ 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
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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team:VM Assigned to OTP team VM
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