The 'threading' CLI option/build param/target property changed to 'single threaded'

[patilatharva]

Todo:

• Change docs accordingly.
• Rename all occurrences of "unthreaded" to "single threaded".
• Add validation check to ensure mutual exclusivity between workers and single-threaded
• Return error upon specifying single-threaded on a federated program.

Also fixes #2080 in the sense that we now adjust paths for any target property that extends FileListProperty.

[cmnrd]

I am not sure if "single threaded" is a good name. I expect that it is difficult for users to see the difference between --single-threaded and --threads=1. Perhaps --unthreaded would be clearer?

[lhstrh]

I am not sure if "single threaded" is a good name. I expect that it is difficult for users to see the difference between --single-threaded and --threads=1. Perhaps --unthreaded would be clearer?

What would actually happen if both --single-threaded and --threads=1 are used?

[cmnrd]

I am not sure if "single threaded" is a good name. I expect that it is difficult for users to see the difference between --single-threaded and --threads=1. Perhaps --unthreaded would be clearer?

What would actually happen if both --single-threaded and --threads=1 are used?

There should be an error message. I am not sure if there is one in place right know, but setting the number of workers in the unthreaded runtime should be an error (or at least a warning). Also using --single-threaded on targets that don't have an unthreaded runtime should be an error.

[petervdonovan]

This looks good to me, as long as other people are happy with the "single-threaded" terminology (there was a discussion on Zulip). Thanks Atharva!

[lhstrh]

Looks good!

The absence of changes to the validator suggests that the combination of the single-threaded and threads target property isn't being checked for, which we should do, as @cmnrd suggested. Would you be able to fix this?

[patilatharva]

The absence of changes to the validator suggests that the combination of the single-threaded and threads target property isn't being checked for, which we should do, as @cmnrd suggested. Would you be able to fix this?

I just added checks in the validator and CLI to make it illegal to set workers when single-threaded is enabled. To clarify, this makes the CLI combination of --single-threaded and --workers=1 illegal, but in LF syntax single-threaded: false, workers: 2 is still allowed.

[edwardalee]

This is nice! However, I'm not sure what workers means. It says it specifies the "default" number of workers, but I would think that the default number of workers would be what you get when you do not specify the workers property. Also, we will now have an inconsistency with the website. I suggest starting a release-4 branch of the lf-website repo and update the documentation in this following locations:

https://www.lf-lang.org/docs/handbook/target-declaration#threading

https://www.lf-lang.org/docs/handbook/target-declaration#workers

https://www.lf-lang.org/docs/handbook/target-declaration#command-line-arguments

https://www.lf-lang.org/docs/handbook/target-language-details?target=c#single-threaded-implementation

https://www.lf-lang.org/docs/handbook/target-language-details?target=c#multithreaded-implementation

[cmnrd]

I just added checks in the validator and CLI to make it illegal to set workers when single-threaded is enabled. To clarify, this makes the CLI combination of --single-threaded and --workers=1 illegal, but in LF syntax single-threaded: false, workers: 2 is still allowed.

What is the reason for allowing single-threaded: false, workers: 2 in the target properties? Shouldn't this be illegal too?

[erlingrj]

Looks good, couple of comments. We should do some renaming in reactor-c also since "unthreaded" is used quite alot

[patilatharva]

What is the reason for allowing single-threaded: false, workers: 2 in the target properties? Shouldn't this be illegal too?

@cmnrd I was thinking it's technically correct to explicitly disable the single-threaded property when multiple workers are specified (even if the single-threaded: false is redundant), but do you think it's better design to just make the two properties mutually exclusive?

[edwardalee]

This looks good to me. I have made some updates in the release-5 branch of the lf-lang website and requested your review to make sure I got it right. Thanks!

[lhstrh]

@patilatharva, I did not yet merge this PR due to bigger changes that were happening in #2008. I will fix this PR after #2008 has been merged. (Expect force pushes on single-threaded.)

[erlingrj]

It seems many of these changes here are not related to moving from threading to single-threaded. They seem to part of the general target-property updates? I did not look closely on them because I think I am lacking that context. Otherwise looks good. But we are still code-generating LF_THREADED and LF_UNTHREADED so I guess we should move to LF_SINGLE_THREADED to be compatible with the reactor-c PR?

[lhstrh]

It seems many of these changes here are not related to moving from threading to single-threaded. They seem to part of the general target-property updates? I did not look closely on them because I think I am lacking that context. Otherwise looks good. But we are still code-generating LF_THREADED and LF_UNTHREADED so I guess we should move to LF_SINGLE_THREADED to be compatible with the reactor-c PR?

Right, there is some overflow from #2008 that crept into this PR, but that's OK; they are closely related and I had to quite drastically alter the code in this PR to accommodate the changes in #2008.

[erlingrj]

@lhstrh, there is a segfault in one of the federated Python programs, not reproducible on my end. Also very hard to see how the seg-fault in Python is due to doing this renaming

[lhstrh]

@lhstrh, there is a segfault in one of the federated Python programs, not reproducible on my end. Also very hard to see how the seg-fault in Python is due to doing this renaming

I also think it's unlikely that this is due to these changes in this PR. Maybe it's related to lf-lang/reactor-c#293.

[lhstrh]

The problem is that our reactor-c CI doesn't run the Python tests...so we don't find out that something is broken until we update the reactor-c submodule pointer in lingua-franca...