-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Multisend #8072
base: master
Are you sure you want to change the base?
Multisend #8072
Conversation
CT Test ResultsNo tests were run for this PR. This is either because the build failed, or the PR is based on a branch without GH actions tests configured. Results for commit 09b1190 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 |
a2f16db
to
3ddff55
Compare
erts/emulator/beam/dist.c
Outdated
if (is_list(to)) { | ||
Eterm pid; | ||
if (context) { | ||
// If we're passing a context, save the state and trap, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should it try to process some of the list before yielding?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to test that this would work with a list of a single pid while ensuring the whole yielding mechanism is correct, so I implemented it this way initially. A good question is how many sends to do before yielding too 🤔
Hi there 👋🏽 I was out for some holidays, but now I'm back and I'll be finding time for this again 🙂 |
This commit implements an initial multisend implementation. The issue here is that it changes the implementation of `send` to be implemented in Erlang instead of directly in C, which incurs a penalty on all regular and existing uses of `send`. This commit originally included changing many preloaded binaries but this have been removed from the commit history in order to comply with the security requirements of PRs to OTP.
If the distribution connection reports that the remote node does not implement `multisend`, we fall back to what regular code would have used instead, so not to change any of the expected guarantees of this send.
This applies the first comments in the review of the original PR, by most importantly keeping the `send` bif implemented in pure C.
3ddff55
to
084745d
Compare
Question, in case somebody knows, I'm having a failure at otp/erts/emulator/test/distribution_SUITE.erl Line 3831 in 81fc1bc
failed_nc_refc_check in particular. Anybody knows what does that mean? 🤔
Edit, I found it has something to do with this: https://github.com/erlang/otp/blob/81fc1bc0a378b7f612415eb8904700b70afc83ce/erts/emulator/beam/erl_node_container_utils.h, now trying to study this new part of the codebase. |
This way we can also remove the port from the call, and keep port knowledge only to the driver.
I've pushed the code that I have that passes the multisend operation "most of the times" for the regular TCP distribution 😅 I'm having two issues, one is the |
My attempt to make #6638 yielding. Much of the points here have been discussed there, most importantly to me @rickard-green 's comment. I've added here two tests, for the regular as well as the TLS distribution, but I've implemented yielding only on the easy case for now and resorted to the original non-yielding code on the hard pure C case (for now). I've also recovered the commits from @devsnek, in order to keep the history of attempts transparent for anyone else that wants to learn about how this was tried, but that is easy to squash. I've also removed commits updating preloaded modules, those need to be updated in order to run tests.
Now, I'm relatively confident tests are decent, but C code, I haven't done any serious C programming in way too long so for now I'm only incredibly happy it stopped crashing 😄 as the old adage says, make it work, then make it beautiful... ok, I think it works, but now all opinions are more than welcome on how to make this beautiful 😄
TODO: