Simulation running slower between 2.14.4 and 2.14.6

[davidhutchens]

The speed of the simulation dropped by about 1/2 between 2.14.4 and 2.14.6. I am running with Java 11.0.2 on a Macbook pro, 15-inch, Early 2013, running macOS Mojava 10.14.2. I tested it with the unmodified jar files for the official releases. In the tests, I made sure that I was using discrete GPU in all cases (The Macbook pro normally automatically switches between the on-chip and separate GPU and that can have an impact on speed). The speed decrease remains through the current develop branch of Logisim-evolution.

[maehne]

Thanks @davidhutchens for narrowing the introduction of the speed regression a bit down! Between 2.14.4 and 2.14.6 are still quite a number of commits. I went through them and found commits 9663688 and 087a804 as likely candidates for having introduced the regression. Could you try to build Logisim-evolution from commit 087a804 and try your test case? Would it be possible to share your test case with us?

[davidhutchens]

@maehne: Sure I can share. It is already available. Go to the same place you downloaded my app earlier: http://cs.millersville.edu/~dhutchens/Logisim/ There is a zip called HardwareExamples.zip. There is a LogisimComputer folder in it containing a number of .circ files. I was running the one called Console.circ. It uses some of the other files in that directory, particularly P6-Computer-IO.circ, S3-Computer-IO.circ, and Parts.circ.

I see that P2-ComputerWithForward.circ also shows the speed difference and it is much simpler. It uses Parts.circ. Those are class examples adding more complexity in each step.

My plan was to start looking at individual commits to further narrow it down when I had time, but I wanted to go ahead and open the issue. Your knowledge of what the commits were doing is a big advantage. If you are going to check those commits you noted above, let me know what you find and what else I should do. Otherwise, I can do it. But we don't need to duplicate the effort.

[davidhutchens]

@maehne : When I checkout either of those commits and try to build it with "ant jar", It gives me a warning: " [javac] warning: [options] bootstrap class path not set in conjunction with -source 7" and then tells me that there are methods on Wire that are not defined: IsSetAsMarked(), GetMarkColor(), SetMarked(). So it didn't compile for me. Perhaps I have done something wrong in checking them out, but I'm not sure what it might be. It does say "race condition fixes" when I checkout the commit. Ideas?

[davidhutchens]

@maehne : I had some time this evening and did a binary search for the commit that slowed things down. Commit 67289e5 runs faster. Commit c69cca5 is slow. Now the question is why.

[maehne]

@davidhutchens: Thanks for providing your test case and looking further into the issue by bisecting the commits to locate the one introducing the regression! The diff of commit c69cca5 gives us a hint what might be the source of the regression: The only file directly involved in simulation is src/com/cburch/logisim/std/arith/Multiplier.java and there the lines 57 to 67 changed quite a bit to calculate the product a * b. These changes were introduced by @BFH-ktt1 to fix issue #129.

Your test case P2-ComputerWithForward.circ contains a multiplier instance in the ExtendedOps sub-circuit. A first check could be to remove this component instance for a test run. Next step could be to undo the modifications in lines 57 to 67 of src/com/cburch/logisim/std/arith/Multiplier.java. If this confirms the source of the regression, we need to think how to speed up that critical code section.

[davidhutchens]

@maehne : I removed the multiplier and divider from the ExtendedOps subcircuit and just set the result to a constant. That was easy because the program in P2 doesn't use that subcircuit since extended ops require a stall that isn't even implemented in P2. The program runs as it always did. Indeed it runs exactly the same, including the slowdown. So the multiplier in the circuit is not involved in the slowdown.

I am confused though. When I do a diff of the two commits above, I don't see Multiplier.java. I do, however see CircuitWires.java, Simulator.java, SimulatorTicker.java, Wire.java, Canvas.java, and CanvasPaintThread.java along with several others that seem less likely to be related to the slowness.

c69cca5 is a merge. Were you doing a diff with the other branch? Maybe I need to detour down that branch to find the specific commit that introduced the slowness?

[maehne]

Thanks @davidhutchens for looking further at this issue. My statement about the Multiplier.java as possible origin came from looking at the diff of c69cca5 on GitHub, but I am seeing the same in my local Git GUI (Sourcetree.app). I agree with you that if the multiplier code is not the source of the regression, looking more closely to the individual commits of the merged branches is needed. Interestingly, the commit graph in my Git GUI shows me that 67289e5 and c69cca5 are on two separate branches with the common ancestor being be4ce63. Only in
804bbbf both branches got merged. It's unfortunate that our Git history is a bit convoluted during this period due we had not yet settled on a standard branching model.

Looking at the slow branch, I come again across the two commits 222fe50 and 3f34004, which touched Simulator.java and SimulatorTicker.java to fix some race condition/crash. The diff suggest that some threading aspects were modified. The other files you mention modified the drawing of the schematic. This might also have an influence on simulation speed.

Note, my remarks are mere educated guesses, I am not yet much familiar with the innards of Logisim nor a long-time Java programmer (I personally prefer much more C++). I am a regular user of Logisim-evolution for my digital design classes and so far I have mostly helped in the development/maintenance by participating in the code reviews of PRs and having contributed some improvements in the packaging of Logisim-evolution for different OS platforms. (That's why I looked into restoring the ability to build a macOS app using gradle.)

[davidhutchens]

@maehne was probably right initially. The commits 9663688 and 087a804 do not compile as I noted above, but if I move out from there to ones that do, i find that be5c89b compiles and runs slowly, but the earlier 52a2f41 compiles and runs faster. The four commits between them do not compile. So the slowdown was introduced after 52a2f41 and before be5c89b. The diff of those two commits is over 700 lines long.

[maehne]

Thanks @davidhutchens for confirming my suspicion. Commits 9663688 and 087a804 were introduced by @alexandremalki into our repository, but the original author of the commits is @kevinawalsh, who maintains the logisim-evolution (Holy Cross Edition). We should look whether in this fork a patch to this regression is already present.

[kevinawalsh]

Yes, that was me. The simulator and GUI threads had (and still have) lots of synchronization issues and race conditions (visible sometimes as the "simulator has crashed" messages). Those two commits fixed some of ones that seemed to often cause failures in practice, improving reliability at the cost of decreased performance.

In kevinawalsh@aae0c4e just a few days ago I actually eliminated most of the locking costs using some very simple optimization. Should be easy for someone to port over to reds-heig. The commits before and after that one further improve simulator performance as well.

[maehne]

Thanks a lot @kevinawalsh for your feedback to this issue and your commits! We will have a look to integrate those patches to fix the regression.

Actually, have you noticed the discussion in issue #215? We are very much pleased that Logisim-evolution seems quite popular as can be seen by the number of forks on GitHub. However, we are worried about the ever growing differences between all those forks as apparently quite some effort is spent on fixing annoying bugs and adding new useful features, but only few of these enhancements get merged back into the original repository. That's a pity, as development of Logisim-evolution could be much more efficient if we manage to work better together.

I checked out your Logisim-evolution Holy Cross Edition and it adds really some neat features! Would you be willing help us integrate them into the official Logisim-evolution version? You forked off your branch quite a while back at Logisim-evolution 2.13.14 in 2015-12-07. Reviewing them in one large pull request for direct merging into our develop branch would risk introducing new bugs into Logisim-evolution due to overseen merge conflicts. Therefore, we would need to bring your changes step by step into the develop by reviewing each time a manageable size of code fixing some issue or adding some feature. So, some work of cherry picking your different changes/enhancements into separate feature branches for review and rebasing onto our current develop branch is needed.

What do you think? It would be best if this discussion would continue over in issue #215.