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

Make lockToCurrentCPU re-entrant #3346

Open
wants to merge 1 commit into
base: master
from
Open

Make lockToCurrentCPU re-entrant #3346

wants to merge 1 commit into from

Conversation

@layus
Copy link
Member

@layus layus commented Feb 6, 2020

Fixes #3345

Copy link

@nmattia nmattia left a comment

This works if the memory is shared, correct? (otherwise didLockCPU will be back to false the second time around). Is the case when performing IFD?

@layus
Copy link
Member Author

@layus layus commented Feb 6, 2020

I guess there could be an issue if some part of the program forks. I think nix may use pthreads, but I do not think it works with multiple processes. In that setup, the above patch works correctly.

The example you describe in #3345 is not an IFD (or did I miss something ?). But you use a builtin that loads local filesystem data (filterSource & related). This works a bit differently than other derivations indeed.

@nmattia
Copy link

@nmattia nmattia commented Feb 6, 2020

It is unfortunately IFD related, if the argument to readDir is e.g. a local directory (and not a derivation) the shell gets the correct number of CPUs.

@layus
Copy link
Member Author

@layus layus commented Feb 6, 2020

Oh, I did not look closely enough.

I did check that it works on the example you provided. So I can tell that it fixes your use case. But I do not know how nix forks and threads itself.

That being said, the comment is quite clear about this being "only" a cpu cache optimization. So the change cat at worst miss an optimization opportunity.

@nmattia
Copy link

@nmattia nmattia commented Feb 12, 2020

@edolstra WDYT?

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

Successfully merging this pull request may close these issues.

2 participants
You can’t perform that action at this time.