-
Notifications
You must be signed in to change notification settings - Fork 63
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
[Experimental] Quasi-static scheduler #1190
base: master
Are you sure you want to change the base?
Conversation
|
||
if (targetConfig.schedulerType == SchedulerOption.QS) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to handle this here with a
if (targetConfig.schedulerType == SchedulerOption.QS) {
targetConfig.compileDefinitions.put("SCHEDULER_QS", "");
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks! This is indeed a cleaner approach.
"threaded/scheduler_" + scheduler + ".c", | ||
"threaded/reactor_threaded.c" | ||
)); | ||
if (scheduler == SchedulerOption.QS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would also be better handled here.
System.out.println(this.main); | ||
System.out.println(main); | ||
|
||
// Generate schedule.h if QS scheduler is used. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry to repeat this, but this should also be in pickScheduler
I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Will revise the code accordingly. Thanks.
@@ -161,6 +166,13 @@ public static String generateInitializeTriggerObjects( | |||
isFederated, | |||
clockSyncIsOn | |||
)); | |||
if (targetConfig.schedulerType == SchedulerOption.QS) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not have this inside generateSchedulerInitializer
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, it makes sense to have this in generateSchedulerInitializer
. Thanks for the catch!
@lhstrh Okay, sounds good to me. |
build.gradle
Outdated
@@ -33,6 +33,9 @@ subprojects { | |||
implementation group: 'com.google.inject', name: 'guice', version: guiceVersion | |||
// https://mvnrepository.com/artifact/commons-cli/commons-cli | |||
implementation group: 'commons-cli', name: 'commons-cli', version: commonsCliVersion | |||
// https://search.maven.org/artifact/tools.aqua/z3-turnkey/4.8.16/jar | |||
implementation 'tools.aqua:z3-turnkey:4.8.16' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that this added dependency is not reflected in the OSGi or Maven configurations. This means that it would only work with lfc
, which is built using Gradle, not with Epoch or the LDS server (and thus VS Code).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For Maven there is this:
<dependency>
<groupId>tools.aqua</groupId>
<artifactId>z3-turnkey</artifactId>
<version>4.8.16</version>
</dependency>
I don't know how to fix the Eclipse configuration, to be honest. But we can't merge this until that is fixed because otherwise the project simply won't build in Eclipse.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe @a-sr has any suggestions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added this to pom.xml
, but Maven still doesn't work. I'm probably missing something obvious again...
But even if Maven works, there appears to be no way for this to work in Eclipse other than building Z3 from source and adding it to the build path. Everyone developing LF in Eclipse would have to do this to get a compiling code base. That doesn't sound great to me. Maybe publish a z3 lib OSGi bundle?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, z3 could be included as a submodule. I expect this kind of problem to surface with other dependencies, too. And including a submodule for every library that isn't available as an OSGi bundle doesn't seem like a good solution either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, all non-plugin dependencies in Eclipse are tricky. If z3 is a compile-time dependency for Java, I guess there is no way around putting it in a plugin to make it work in Eclipse.
We also had this issue in Klighd with the Sprotty library that is only available on maven central. We created a repository to wrap it into a plugin (sprotty-mirror). The build automatically pulls the library, builds it into a plugin, and deploys it as an update site, such that we can use it in our Eclipse and Oomph setup as dependency. Maybe we need something similar for LF.
The official Eclipse Orbrit works with the same principle.
Regarding the dependency you added to the pom file, I do not know why this does not work or if that even will work. In the maven configuration, the option to consider pom dependencies in addition to the target platform is enables but I do not know how it works in detail. The current configuration has something to do with the workaround for Kotlin, but to be honest I am not sure how all these configurations work together.
Sorry.
THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. | ||
***************/ | ||
|
||
package org.lflang.generator.c; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incorrect package declaration. That said, I'm not sure this is the right place to put this generator. It generates UCLID code, so I think it should be in a separate uclid
package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Will put that in a separate uclid
package.
@Soroosh129 brought up the issue that naming the implementation a "quasi-static scheduler" might be too generic, since there can be different variants of the quasi-static scheduler (e.g. a QS scheduler with a schedule generation mechanism using some other constraint solvers). When specifying the QS scheduler, it would be useful to further parametrize the scheduler. For example, |
This strikes me as excess generality. Wouldn't something like |
f39097d
to
407436c
Compare
407436c
to
81876ac
Compare
81876ac
to
f19ec04
Compare
This PR implements a quasi-static (QS) scheduler proposed in #1080. The QS scheduler generates a fixed static schedule at compile time and follows the fixed schedule at runtime.
To use this scheduler, the user needs to install two additional dependencies: Uclid5 and z3. Both need to be added to PATH and able to be invoked on the command line.
The scheduler is currently experimental (i.e. not production-ready). For small test cases such as
ThreadedThreaded.lf
, it can achieve comparable performance wrt that of the default NP scheduler. However, for large benchmarks such asSleepingBarber.lf
, the QS scheduler currently exhibits a significant slowdown. We are yet to figure out the cause of this.