Redrawing of GUI-windows (Close #786)

[lorenzonotaro]

Hi everyone,

While investigating the performance issues reported in different occasions:

• Redrawing of GUI-windows #786
• Evolution runs slower than Logisim even at max speed on my pc, is that normal? #1493
• Clock Frequency, not reaching 2048K #1544

using the IntelliJ profiler I have found the following:

1. The Simulation Thread spends ~17% of its CPU time on a SwingUtilities.invokeAndWait call in com.cburch.logisim.circuit.CircuitWires#getBundleMap.
   As the comment above the function explains, the bundle map must be computed by the AWT thread only: if the calling thread is anything other thant the AWT thread, the call is placed at the end of the AWT event queue and the result is then awaited and returned.

The Simulator repeatedly calls getBundleMap() in its propagate() method, and each time it has to wait for the AWT thread to consume the entire event queue before the simulation can continue.
As the comment above CircuitWires.getBundleMap() explains, in the AWT thread section the function checks whether masterBundleMap is set and if it is (i.e. it hasn't been invalidated), it just returns that instance. By moving this check at the very beginning of the function, this value can be returned instantly to the calling simulation thread, without piling on the AWT Event Queue and waiting for it to be extinguished just to return a precomputed value.

2. The event handler com.cburch.logisim.gui.main.Canvas#propagationCompleted, called by the simulation thread as shown here effectively blocks the simulation thread until redrawing is completed. As suggested here, the propagationCompleted event handlers could be called from the AWT thread, but I tried this and the event queue gets flooded with these events and the GUI quickly freezes. Instead, my solution is to simply remove the line in the handler that makes the simulator wait for the repaint to be done: I couldn't understand why does the simulator needs to wait in the first place.

By making these two changes, I managed to increase the tick speed of the project I used for testing (my own design of a 8-bit processor) from ~400 Hz before the changes to ~1.2kHz after the changes.

[davidhutchens]

This speeds up my simulations by better than a factor of 2. It looks reasonable to me, but I'm not sure if it is actually thread safe. @BFH-ktt1 has worked more with with the simulator and should have a look.

[davidhutchens]

After some thought, I'm pretty sure that this is not thread safe as written. In the getBundleMap code take the following sequence. First the simulation thread arrives and checks if masterBundleMap is null and finds it is not. Then the AWT thread nulls the masterBundleMap (it has a method for that purpose). Then the simulation thread returns the now null value of masterBundleMap. I strongly suspect bad things to happen if this method returns null.

This could potentially be fixed by assigning masterBundleMap to a local variable, comparing that local variable to null and returning that local variable if not null. This essentially gives an atomic fetch of a single value.

Granted, the AWT thread could set masterBundleMap to null while the simulation is using the one it gets, but that seems to me to have always been the case.

Again, @BFH-ktt1 should examine this as there is likely more going on than I am aware of.

[davidhutchens]

As to change #2, The simulation likely waits for the draw because if it continues it will update the values while the draw is in process and the resulting drawing will not represent a valid state. This might matter at a slow clock rate where things can be examined on screen.

However, if the clock is ticking faster than the user can examine the diagram, that might not matter. What I am suggesting is: can we keep the wait for a slow clock and skip it with a fast clock?

[BFH-ktt1]

I'm not very familiar with the simulator engine. However, @kevinawalsh did a lot of work on the simulation engine to speed it up. He worked also on the repainting, so maybe we should look how he solved this problem in his holycross version and "cherry-pick" to merge his great work into evolution.

[davidhutchens]

In @kevinawalsh's CircuitWire code, line 1055, There is this:
/*synchronized*/ private Connectivity getConnectivity() {
Connectivity ret = masterConnectivity; // volatile read by AWT or simulation thread
if (ret != null)
return ret;
if (SwingUtilities.isEventDispatchThread()) {
// AWT event thread.

He does, with the local variable ret, exactly what I suggested above for change 1. This change gives most of the speedup. It was in commit aae0c4e.

[davidhutchens]

In @kevinawalsh's Canvas code, line 446, There is this:

@Override
public void propagationCompleted(Simulator.Event e) {
paintCoordinator.requestRepaint();
}

So he has done the precise change that this PR suggests for Canvas. He did that last month in this commit: 4ca7a39.

Note that his commits had more changes than just these, so I don't know if they are safe by themselves.

[BFH-ktt1]

@davidhutchens : Thanks for looking it up! I would suggest that we "cherry pick" those two commits instead of only changing the two lines like in this PR. What do you think?

[davidhutchens]

@BFH-ktt1 : I agree that it would be best to take advantage of that careful work. For example, in sleeping on it, I realized that one of those "more changes" was to declare masterBundleMap (later renamed as masterConnectivity) as volatile. It is easy to make mistakes in inter-thread communication.