Skip to content

[RFE] use binary instead of atom for lcnt lock name #9388

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

Closed
wants to merge 1 commit into from

Conversation

zzydxm
Copy link
Contributor

@zzydxm zzydxm commented Feb 4, 2025

We met an issue when using lcnt on a node with millions of active sockets. It has to create 2 atoms like 'esock.r[xxxxx]' for every socket, so that the lcnt is locking itself on the atom table.

This change makes the lcnt use binaries instead of atom as lock name to avoid this issue.

However, this change slows down the lcnt result processing and also makes it use a lot more memory. Just raising this PR for discussion.

Copy link
Contributor

github-actions bot commented Feb 4, 2025

CT Test Results

    4 files    152 suites   51m 59s ⏱️
1 723 tests 1 673 ✅ 50 💤 0 ❌
2 448 runs  2 378 ✅ 70 💤 0 ❌

Results for commit 2dc1a68.

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

@bmk
Copy link
Contributor

bmk commented Feb 5, 2025

I will look into making it possible to simplifying the socket mutex names (from "esock.r[10]" to just esock.r").

@zzydxm
Copy link
Contributor Author

zzydxm commented Feb 5, 2025

can we add an id field to ErlDrvMutex_ and make MCREATE support 2 args? in that way we don't need to do
sprintf(buf, "esock.w[" SOCKET_FORMAT_STR "]", sock);
descP->writeMtx = MCREATE(buf);

@jhogberg
Copy link
Contributor

jhogberg commented Feb 6, 2025

If you’re referring to id as present in the lock counting output, no. It has to be an immediate, and we’re careful not to expose any distinction between immediates and boxed terms in the NIF and driver interfaces.

@michalmuskala
Copy link
Contributor

One option could be to have esock.r as the "type" and then an integer for the name - but this would require changes to the API to support this.

@jhogberg
Copy link
Contributor

jhogberg commented Feb 6, 2025

One option could be to have esock.r as the "type" and then an integer for the name - but this would require changes to the API to support this.

That still leaks a difference between small and big integers. 😕

@michalmuskala
Copy link
Contributor

I guess if the argument is limited to a small than we could guarantee it's an immediate across all runtimes, but the size could be quite limiting. Otherwise we'd need to document maximum allowed value for the integer - still not ideal

@zzydxm
Copy link
Contributor Author

zzydxm commented Feb 6, 2025

I will look into making it possible to simplifying the socket mutex names (from "esock.r[10]" to just esock.r").

Do you mean you will do some other changes or just remove the SOCKET_FORMAT_STR on mutex creation? I can do that

@bmk
Copy link
Contributor

bmk commented Feb 7, 2025

The solution I have now is that the "format" of the mutex name can be configured.
That is a 'verbose' format (as today) or a simplified format: Just the "esock.r":
--enable-esock-verbose-mtx-name | --disable-esock-verbose-mtx-name

@rickard-green rickard-green added the team:VM Assigned to OTP team VM label Feb 10, 2025
@jhogberg jhogberg added team:PS Assigned to OTP team PS and removed team:VM Assigned to OTP team VM labels Feb 10, 2025
@rickard-green rickard-green added team:VM Assigned to OTP team VM and removed team:PS Assigned to OTP team PS labels Feb 10, 2025
@jhogberg
Copy link
Contributor

I'm closing this PR as @bmk will fix the underlying issue.

@jhogberg jhogberg closed this Feb 10, 2025
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.

5 participants