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

refactor: More explicit NativeConfig.multithreading setting #3811

Merged

Conversation

WojciechMazur
Copy link
Contributor

  • Rename multithreadingSupport to multithreading
  • Make NativeThread.multithreading Option[Boolean], defaulting to None. When not defined assume multithreading is enabled, but detect if threads are used. If not, relink with disabled multithreading to improve performance.
  • Remove control env variable SCALANATIVE_DISABLE_UNUSED_MULTITHREADING

@WojciechMazur
Copy link
Contributor Author

@armanbilge AFAIR you didn't like the previous detection, do you think that making it more explicit and not controlled by env variables might be beneficial here? I wanted to have a good balance between performance and development experience.
Now if setting can be empty the behaviour shall not be so much suprising.

@armanbilge
Copy link
Member

AFAIR you didn't like the previous detection

Hmm, sorry, not sure if I remember 😅 I remember talking about ENV variables in #3517 (comment) and in #2800 (comment).

Still, I agree this is better.

If not, relink with disabled multithreading to improve performance.

Sorry if I missed it, is that in this PR? Does it log a warning to the user? Linking twice seems expensive.

@WojciechMazur
Copy link
Contributor Author

If not, relink with disabled multithreading to improve performance.

Sorry if I missed it, is that in this PR? Does it log a warning to the user? Linking twice seems expensive.

This mechanism was already added, only thing that changed in this PR was the control over when to perform detection of threads usage. However, maybe I've used wrong names here - we detect usage of threads after initial class loading (loading NIR from files + its post processing).
In general cost of class-loading from disk it relatively low, especially when compared to possible performance gains. At that point we have both warmed up JVM and cached NIR read from disk.
First classloading of our tests takes ~4s and the whole linking process ~97s. In case of simple sandbox apps, first load is ~1s, and second one with disabled multithreading ~0.5s

@armanbilge
Copy link
Member

That makes more sense. Thanks for clarifying!

@WojciechMazur WojciechMazur merged commit 449bce6 into scala-native:main Mar 6, 2024
61 checks passed
@WojciechMazur WojciechMazur deleted the refactor/multithreadingSetting branch March 6, 2024 09:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants