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

Replace the 'threads' target property with the 'workers' and 'threading' properties #993

Merged
merged 42 commits into from
Mar 10, 2022

Conversation

cmnrd
Copy link
Collaborator

@cmnrd cmnrd commented Mar 2, 2022

This PR implements the solution discussed in #290.

Based on our discussion yesterday, the proposal is to have the following two target properties: threading = on | off (if supported, default is on) and workers = [int]. We will deprecate threads because it is ambiguous and its meaning overloaded. By the time that we end up implementing preemption (where we the size of the thread pool has to outnumber the workers), we could add an extra parameter for that (may be thread-pool-size = [int]).

I implemented the change for all the targets. However, there are some remaining issues that I don't know how to resolve.:

  • The number of worker threads is configured in various places throughout the C generator and even in duplicated code in the Python generator (see the diff). In the Python generator, even contradicting values are assigned. This is a large code smell to me, but I don't know how to fix it.
  • Before, the unthreaded runtime has been the default. Now, threading=true and workers=0 are the default. I don't know if the C runtime will interpret workers=0 correctly, as before it was not possible to have the threaded runtime with zero workers configured.
  • Since the default changed, this influences all the C tests that don't configure threads. Should we set threading=false in all tests that don't specify something else?
  • In org.lflang.tests.Configurators the method useFourThreads reconfigures a test to use 4 workers. Should we force enable threading in this case?
  • Remove workers: N entirely from all of our tests
  • Test the workers property (done in C and C++)

Unrelated to the above, this PR also fixes #1020.

@lhstrh
Copy link
Member

lhstrh commented Mar 2, 2022

* Since the default changed, this influences all the C tests that don't configure threads. Should we set `threading=false` in all tests that don't specify something else?

* In `org.lflang.tests.Configurators` the method `useFourThreads` reconfigures a test to use 4 workers. Should we force enable threading in this case?

I think we can either specify threading=false in all the generic tests, or we can leave it out and instead use a useSingleThread configurator to impose that property (instead of useFourThreads). The goal is to ensure that all tests are run using both runtime implementation. What is the default value of workers? I think it would make sense to match the number of available cores by default.

@cmnrd
Copy link
Collaborator Author

cmnrd commented Mar 3, 2022

We don't know the number of available cores on the target machine. In fact, the same binary could be run on different machines.

To quote from the comment in TargetConfig.java:

The number of worker threads to deploy. The default is zero, which indicates that the runtime is allowed to freely choose the number of workers.

I think this is the most reasonable default and it is also what is currently implemented for C++ (and I think for Rust). For zero workers, the C++ runtime uses the number of hardware threads available on the executing machine.

@Soroosh129
Copy link
Contributor

Soroosh129 commented Mar 3, 2022

For zero workers, the C++ runtime uses the number of hardware threads available on the executing machine.

I don’t know any reliable method for this that works on all platforms for C. Do you happen to know any? Maybe CMake has something?

@cmnrd
Copy link
Collaborator Author

cmnrd commented Mar 3, 2022

No, I don't know. But it is also not needed to implement the exact same strategy for C. To me, the key is that workers=0 means unspecified. This gives the targets the freedom to implement whatever strategy seems feasible. It could use a hard-coded number of workers (e.g. 4)., it could use the numbers of cores of the machine executing the compiler, it could check the number of cores at runtime, or use some heuristic to identify the best number of workers for a given application.

@Soroosh129
Copy link
Contributor

[E]ven in duplicated code in the Python generator

The code duplication in generatePreamble is going away in #1009.

@lhstrh
Copy link
Member

lhstrh commented Mar 9, 2022

I don't know if the C runtime will interpret workers=0 correctly, as before it was not possible to have the threaded runtime with zero workers configured.

It doesn't as far as I know. As I asked above, I'm not sure how we should interpret workers=0. If we need to determine the number of CPU cores, we have a huge task ahead of us.

How about a default value >1 (maybe 4 or 8?) in lieu of a mechanism that detects the number of cores? That said, I don't think it would be a "huge task" to determine the number of cores but it will require some care to craft a mechanism that works reliably across platforms. In this case, too, I would say the default number is a reasonable fallback option.

EDIT: I hadn't noticed that @cmnrd already suggested the same solution. @Soroosh129, if you agree, could you push a fix to reactor-c? Thanks!

@Soroosh129
Copy link
Contributor

Now, threading=true and workers=0 are the default.

Is this just limited to tests?

It occurs to me that this might not be the best default in general for the Python target.
There is no true multi-threaded concurrency in Python.
Setting threading=true will add a lot of overhead by using the threaded reactor-c and requiring GIL locks in the body of reactions.

Maybe the same is true for the TypeScript target?

@Soroosh129
Copy link
Contributor

We still have a weird case for federated execution.

As mentioned before, the CGenerator attempts to adjust the number of workers to account for all the blocking network input control reactions (there should be at least one worker available for each at all times). This is simply done by adding the number of network inputs to the number of workers during code generation.

The problem is that the C runtime will see a non-zero number of workers for federated programs, even if workers=0.

What do you think we should do here? The solution to this is not very clear to me.

Copy link
Member

@lhstrh lhstrh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! Thanks!

org.lflang.tests/src/org/lflang/tests/Configurators.java Outdated Show resolved Hide resolved
example/C/src/Deadline.lf Outdated Show resolved Hide resolved
example/C/src/Patterns/Chain_02_Pipeline.lf Show resolved Hide resolved
example/C/src/ROS/ROSBuiltInSerialization.lf Outdated Show resolved Hide resolved
example/C/src/ReflexGame.lf Outdated Show resolved Hide resolved
example/C/src/ReflexGame/ReflexGameTest.lf Outdated Show resolved Hide resolved
example/C/src/Rhythm/Rhythm.lf Outdated Show resolved Hide resolved
example/C/src/Rhythm/SensorSimulator.lf Outdated Show resolved Hide resolved
example/C/src/TrainDoor/TrainDoor.lf Outdated Show resolved Hide resolved
@lhstrh lhstrh merged commit 87f8bab into master Mar 10, 2022
@lhstrh lhstrh deleted the workers branch March 10, 2022 04:53
@lhstrh lhstrh added the enhancement Enhancement of existing feature label Mar 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement of existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

List modal reactor tests in own test category
3 participants