Smarter default number of workers in C runtime

[erlingrj]

I thought I'd address some minor issues as a way of familiarizing myself with the codebase.

This is adressing issue with a smarter default number of workers #1096. Currently only implemented for C. In case the user does not specify the number of worker threads, we use the minimum of the number of cores and the breadth of the Reaction graph. With this proposal the C (and in the future the C++ and Rust) runtime will always be configured with a specific number of workers.

Questions:

1. Is it a reliable approach to find number of cores through Java runtime?
2. Could this be done higher in the class hiererachy? Such that we dont have to reimplement it for all target languages? A problem I saw is that the ReactionInstanceGraph does not seem to be built in the abstract GeneratorBase class but rather in the derived classes
3. In this approach we will suggest 4 threads if there is a single Reactor with 4 reactions. This might not be what we want as they will never execute in concurrently.

When questions are resolved I can port to the other target language generators

[Soroosh129]

Hi @erlingrj

Thanks for submitting this PR.

The threaded C runtime determines the number of workers at runtime. I personally prefer the existing approach because we use the number of cores of the machine that executes the program, not the number of cores of the machine that code generates the program.

In keeping with the current scheme of things, the calculated breadth would simply have to be conveyed to the C runtime.

But, if you think we should determine the number of cores statically at code generation time, we should remove the duplicate logic in the C runtime.

[erlingrj]

Another alternative would be to pass the breadth as a flag through CMake. Then the runtimes could do the comparison of available cores and breadth of reaction graph themselves.

In my opinion, it would be better to not do the reaction-graph analysis during run-time. But I think you have a more qualified opinion than me

[Soroosh129]

That sounds like a great strategy.

I think your code can slightly be adjusted to accommodate this design. In createMainReactorInstance, you could add the following logic:

if (this.mainDef != null) {
    if (this.main == null) {
        // Recursively build instances. This is done once because
        // it is the same for all federates.
        this.main = new ReactorInstance(toDefinition(mainDef.getReactorClass()), errorReporter, 
            this.unorderedReactions);
+       var reactionInstanceGraph = this.main.assignLevels();
-       if (this.main.assignLevels().nodeCount() > 0) {
+       if (reactionInstanceGraph.nodeCount() > 0) {
            errorReporter.reportError("Main reactor has causality cycles. Skipping code generation.");
            return;
        }
+       if (targetConfig.workers == 0) {
+           targetConfig.compileDefinitions.put( 
+               "LF_REACTION_GRAPH_BREADTH",   
+                reactionInstanceGraph.getMaxBreadth()
+           )
+       }
        // Force reconstruction of dependence information.
        if (isFederated) {
            // Avoid compile errors by removing disconnected network ports.
            // This must be done after assigning levels.  
            removeRemoteFederateConnectionPorts(main);
            // There will be AST transformations that invalidate some info
            // cached in ReactorInstance.
            this.main.clearCaches(false);                    
        }
    }   
}

The C runtime can then take LF_REACTION_GRAPH_BREADTH into consideration in determine_number_of_workers (similar to the way it considers NUMBER_OF_WORKERS).

[edwardalee]

You say:

3. In this approach we will suggest 4 threads if there is a single Reactor with 4 reactions. This might not be what we want as they will never execute in concurrently.

But if the reaction graph breadth is getting calculated correctly, the breadth should be 1 in this case.

[erlingrj]

I added your suggestion @Soroosh129. There was some confusion that maybe you can help me with.

What is it that ReactionInstanceGraph is referring to with numReactionsAtLevel? I am not really sure what it is counting, but it is not what I expected. To find the breadth of the graph I added my own logic into the assignLevels functions. And count the breadth with "numIndependentReactionsPerLevel".

An example of this is the following graph:

Where numReactionsPerLevel= [3,2,2,1]
and my newly added numIndependentReactionsPerLevel=[1,2,2,1]

[edwardalee]

In this example, I'm pretty sure what we want is numIndependentReactionsPerLevel=[1,2,2,1].

[Soroosh129]

[3, 2, 2, 1] sounds incorrect to me as well.

I think numIndependentReactionsPerLevel is actually what we want. Would it be possible to merge these two logic (or get rid of the old one)?

[erlingrj]

I will look into that

[erlingrj]

I merged them now. It works on a few examples here.

[Soroosh129]

Looks good to me! Thank you for taking care of this issue.

We should perhaps do a performance comparison before and after this change but I don’t think that should block merging this in.

[Soroosh129]

Nice catch! Yes, I think there is no need to check the number of workers here.

[Soroosh129]

It looks like the indentation is incorrect.

[Soroosh129]

By the way, I think it might be better if the necessary changes to reactor-c were also a part of this PR for the sake of completeness.

[erlingrj]

How do you structure PRs that include multiple git-repos? I have made a PR in reactor-c with the same name here

[Soroosh129]

How do you structure PRs that include multiple git-repos? I have made a PR in reactor-c with the same name here

You can update the hash that is pointing to a particular commit in reactor-c via a git add org.lflang/src/lib/c/reactor-c and a git commit -m “Updated reactor-c”.

[lhstrh]

This looks good, but I see some conflicts arose in the meantime. If you can fix these, we can quickly go ahead and merge this in. Thanks!

[erlingrj]

I merged the the PR locally.