Scalability part I: Multiports #759
Conversation
…nstream destinations
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.
…guaFrancaShapeExtensions.xtend Co-authored-by: Marten Lohstroh <[email protected]>
…guaFrancaShapeExtensions.xtend Co-authored-by: Marten Lohstroh <[email protected]>
Co-authored-by: Soroush Bateni <[email protected]>
Co-authored-by: Soroush Bateni <[email protected]>
Co-authored-by: Soroush Bateni <[email protected]>
Co-authored-by: Soroush Bateni <[email protected]>
Co-authored-by: Soroush Bateni <[email protected]>
| * @author{Marten Lohstroh <[email protected]>} | ||
| * @author{Edward A. Lee <[email protected]>} | ||
| */ | ||
| public class ReactorInstance extends NamedInstance<Instantiation> { |
There was a problem hiding this comment.
This class is huge. A good rule of thumb seems to be to keep classes below 500 lines. Is there maybe a way to split this class up. Also several methods in this class are too long (The method on line 911 for instance). Generally, method bodies should fit on a single screen.
|
BTW: There is something fishy going with the history in this branch. The commits between As for the effectiveness of the changes made, I did a quick performance test compiling the bank transaction benchmark with 1000 accounts. It looks like this branch improves execution time, but increases the memory footprint even further. |
|
As for the fishy history, I have no idea... All I did was merge in master. Was this the wrong thing to do? |
|
The interleaved connection in Banking foils the methods used in this PR, I think. It will require a bit more work to handle those efficiently. |
Method was renamed from containsReaction to contains.
Merging master shouldn't be a problem. But it looks like what actually happened is not a merge, but a rebase (there is also no merge commit). I observed this happening a few times now (we also had this in one of Marten's branches), but I don't know what causes this. Consider for instance the commit ce52695 on master. It is the same as commit 631e261 on this branch, but they got different hashes. The two commits are duplicates. I don't know what is causing this though... How did you perform the merge? Directly with |
|
I used |
|
Oh, that explains it. When you are merging, you should use |
|
Ugh, ok. Does this need to be undone? If so, how? |
|
No, I don't think that we need to fix it. Nothing actually broke, we will only have a few duplicated commits when we merge this into master. For fixing it we would need to rewrite the history of this branch, but his would come at the high risk of breaking something else. |
|
I want to be extra clear here to avoid further misunderstandings. When you do a @edwardalee: I'm glad we were able to pinpoint this, because it likely also explains some other issues that you have been experiencing with Git that really puzzled me. |
This branch does a major refactoring of the instance classes, ReactorInstance, ReactionInstance, PortInstance, etc., converting them all to Java, eliminating the MultiportInstance class, and consolidating so that a multiport results in exactly one PortInstance being created rather than one PortInstance per channel. This is a large PR with a lot of changes, but it is still a work in progress. Quite a bit more work needs to be done, most notably to handle banks more intelligently and create only one ReactorInstance per bank rather than one ReactorInstance per bank member. That additional work will occur in a new branch derived from this one.