-
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
Scalability banks #875
Scalability banks #875
Conversation
…r all reactors have been instantiated, applied a common naming convention, and used the array of self struct pointers to solve scoping problems.
…d initializing reactors.
Co-authored-by: Peter Donovan <[email protected]>
This is based on a suggestion from Christian, since the Python runner script does not parse the output of the modified benchmark correctly. The original benchmark has a couple of minor corrections that also appear in the one that uses the benchmark runner.
Adding a reference to #843 so we don't loose track of conversations pertaining to this PR that happened earlier. |
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've browsed through the changes and have seen a ton of code improvements (aside from the prime goal of this PR, to address the algorithmic complexity problems). Given the large size of this change set, I haven't inspected everything very carefully, but my overall sense is that this is a huge overall improvement, and given that the tests all pass, we should merge this in.
org.lflang.tests/src/org/lflang/tests/compiler/MixedRadixIntTest.java
Outdated
Show resolved
Hide resolved
org.lflang.tests/src/org/lflang/tests/compiler/MixedRadixIntTest.java
Outdated
Show resolved
Hide resolved
org.lflang.tests/src/org/lflang/tests/compiler/MixedRadixIntTest.java
Outdated
Show resolved
Hide resolved
org.lflang.tests/src/org/lflang/tests/compiler/MixedRadixIntTest.java
Outdated
Show resolved
Hide resolved
org.lflang.tests/src/org/lflang/tests/compiler/MixedRadixIntTest.java
Outdated
Show resolved
Hide resolved
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.
Looks great! While I was not able to do a detailed review on this, I agree with Marten that this looks like a major improvement. That the time spent in c-benchmark-tests
in the CI runs went down from a few hours to a few minutes speaks for itself.
After merging this, we should put it to the test and revisit the bank transaction benchmark (#624)
// Find name of current project | ||
String id = "((:?[a-z]|[A-Z]|_\\w)*)"; | ||
Pattern pattern; | ||
if (File.separator.equals("/")) { // Linux/Mac file separator | ||
pattern = Pattern.compile( | ||
"platform:" + File.separator + "resource" + File.separator + id + File.separator); | ||
} else { // Windows file separator | ||
pattern = Pattern.compile( | ||
"platform:" + File.separator + File.separator + "resource" + File.separator + File.separator + | ||
id + File.separator + File.separator); | ||
} | ||
// FIXME: If we have to hunt through generated code to find this information, then maybe that's a sign | ||
// that our left hand isn't talking to our right. | ||
Matcher matcher = pattern.matcher(code); | ||
String projName = ""; | ||
if (matcher.find()) { | ||
projName = matcher.group(1); | ||
} | ||
try { | ||
IResource[] members = ResourcesPlugin.getWorkspace().getRoot().members(); | ||
for (IResource member : members) { | ||
// Refresh current project, or simply entire workspace if project name was not found | ||
if (projName.isEmpty() || projName.equals(member.getFullPath().toString().substring(1))) { | ||
member.refreshLocal(IResource.DEPTH_INFINITE, null); | ||
System.out.println("Refreshed " + member.getFullPath()); | ||
} | ||
} | ||
} catch (IllegalStateException | CoreException e) { | ||
System.err.println("Unable to refresh workspace: " + e); | ||
} | ||
} |
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.
Extracting some location pattern from the generated code seems quite complicated.
If it is sufficient to update only the project the source LF file is located in, assuming that the generated files will be in the same project, it can be done via the Resource that is available in the generator.
URI uri = resource.getURI();
if (uri.isPlatformResource()) { // This condition should normally be met when running Epoch
IResource member = ResourcesPlugin.getWorkspace().getRoot().findMember(uri.toPlatformString(true));
member.getProject().refreshLocal(IResource.DEPTH_INFINITE, null);
}
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 is a great suggestion. I've made a note to try this out after we merge this PR, since it's pretty orthogonal to the PR, and it will require manual testing since we don't have regression tests for Eclipse usability features like this. The above code is new only because it was converted from xtend to Java.
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.
As far as I was able to assess it, the changes look good to me. I wasn't able to review every detail and interconnection but I concur with the others that this PR will bring major improvements.
I already spotted a few segments that might get a bit tricky when merging into my modal models branch but I think it won't be that hard.
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.
Looks good to me as well!
I skimmed through this PR and I agree that the code base is substantially improved in this PR.
My experience in bringing these changes over to the PythonGenerator in #873 also proved to me that this new design for banks is much easier to comprehend and work with.
Good suggestion. Co-authored-by: Marten Lohstroh <[email protected]>
Fixed typo. Co-authored-by: Marten Lohstroh <[email protected]>
This branch redesigns the intermediate form (ReactorInstance and related classes) that is used by the diagram generator, the cycle analysis code, and the C code generator. In this redesign, each instance of the class ReactorInstance (and related classes) represents one or more runtime instances of the reactor. If the reactor is instantiated as a bank and/or is instantiated within a reactor that is instantiated as a bank, then one ReactorInstance object represents all bank members. The totalWidth of the ReactorInstance is the total number of runtime instances. The RuntimeRange class represents a contiguous range of such instances, and the CGenerator generates code that iterates over such ranges. A numbering system for the runtime instances based on permuted mixed-radix numbers supports such iteration. The PortInstance class, in addition to representing multiple instances within nested banks, also represents multiple channels (if it is a multiport), and the RuntimeRange class supports iterating over contiguous ranges of channels.
There are still large parts of CGenerator and GeneratorBase that need quite a bit of work, but these are largely orthogonal the focus of this pull request and hence should probably be postponed to another pull request.
With apologies to @a-sr, this redesign may be hard to absorb into the modal models branch. I'm happy to help with that.