RFR: Enhancements for Shenandoah's generational mode [v5]

[Roman Kennke]

On Mon, 13 Feb 2023 22:59:38 GMT, William Kemper <wkemper at openjdk.org> wrote:

>> During the course of the development of the generational mode for Shenandoah, we have found this tool useful to demonstrate progress and changes in behavior. We have also found this tool useful for troubleshooting performance issues and debugging crashes. There are many changes here, but these are the highlights:
>> * The age and affiliation of a region are encoded in the border and shape of the region (respectively).
>> * Phases are encoded with different colors for different generations and whether they have degenerated.
>> * A mechanism to record and replay session has been added (record feature is implemented in hotspot and is not yet upstream).
>> * Popup windows can be opened for additional detail on regions, as well as their history.
>> * The legend shows the number of regions in the state described by the legend item.
>> * Visualizer can now 'find' VMs running Shenandoah with region sampling enabled.
>> 
>> Many months ago we broke backward compatibility on our branch, but we have recently restored it so the time seems right for a PR. Thank you for looking and sorry for the massive number of changes.
>
> William Kemper has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Add notes for running with JDK17+

Hi William,
thank you for this! Awesome work. I have some comments, questions and suggestions.

README.md line 49:

> 47: Add this additional flag to an active JVM running Shenandoah:
> 48: 
> 49:     $ -Xlog:gc+region=debug:<file name>::filesize=<Target byte size for log rotation>,filecount=<Number of files to keep in rotation>

I am not convinced that unified logging is the right vehicle for recording visualizer region sampling information.

src/main/java/org/openjdk/shenandoah/DataLogProvider.java line 2:

> 1: /*
> 2:  * ====

This copyright notice looks wrong. Also, update copyright notices in all files that you have changed.

src/main/java/org/openjdk/shenandoah/DataProvider.java line 85:

> 83:     private static <T> T getMonitor(MonitoredVm vm, String key) {
> 84:         try {
> 85:             //noinspection unchecked

What is this comment trying to say?

src/main/java/org/openjdk/shenandoah/RegionStat.java line 76:

> 74:         this.showLivenessDetail = Boolean.getBoolean("show.liveness");
> 75:     }
> 76:     //This constructor is for CounterTest

Please add space after //

src/main/java/org/openjdk/shenandoah/RegionStat.java line 191:

> 189:                         g.setColor(mixAlpha(TLAB_ALLOC, liveLvl));
> 190:                         fillShape(g, lx, y, tlabWidth, height);
> 191: //                        g.setColor(TLAB_ALLOC_BORDER);

No need to keep commented-out code here and elsewhere.

src/main/java/org/openjdk/shenandoah/ShenandoahVisualizer.java line 67:

> 65: 
> 66:     public static void main(String[] args) throws Exception {
> 67:         // Command line argument parsing

This method is way too big for my taste, and has way too many inline/anonymous classes. This could benefit from some refactoring and better structuring, IMO.

src/main/java/org/openjdk/shenandoah/ShenandoahVisualizer.java line 85:

> 83:             @Override
> 84:             public void paint(Graphics g) {
> 85:                 render.renderGraph(g);

I know that this is pre-existing (afaict), but I would strongly recommend to not render the complex stuff directly on the frame. In my experience, the animation goes much smoother (less flickering, better performance, less blocking of event dispatch thread), if the scene is rendered on an offscreen image (using a separate thread), and then, when redrawing is requested on the frame (possibly requested by the render thread, or whenever the GUI thinks it needs to redraw), only draw the offscreen image on the frame. (This could be further improved by not actually drawing on the offscreen image on the frame, but instead flip the buffers, see here for more details on this: https://docs.oracle.com/javase/tutorial/extra/fullscreen/doublebuf.html .) Maybe this should be done as a follow-up change, and not here, though,

src/main/java/org/openjdk/shenandoah/ShenandoahVisualizer.java line 188:

> 186:                     toolbarPanel.setLastActionField("File selected: " + filePath[0]);
> 187: 
> 188:                     System.out.println("Selected file: " + filePath[0]);

Is this debug output or is this useful? Not sure.

src/main/java/org/openjdk/shenandoah/ShenandoahVisualizer.java line 284:

> 282:         toolbarPanel.setSpeedSpinnerListener(speedSpinnerListener);
> 283: 
> 284:         ActionListener speed_0_5_Listener = new ActionListener() {

That seems an odd variable name (format). Maybe halfSpeedListener ?

src/main/java/org/openjdk/shenandoah/ShenandoahVisualizer.java line 298:

> 296:         toolbarPanel.setSpeed_0_5_Listener(speed_0_5_Listener);
> 297: 
> 298:         ActionListener speed_2_Listener = new ActionListener() {

doubleSpeedListener?

src/main/java/org/openjdk/shenandoah/ShenandoahVisualizer.java line 449:

> 447:             }
> 448:         });
> 449:         f[0].get();

Regarding the refactoring again, it seems odd that you have a final local array field here, only to allow accessing the first element. I feel that this should go into a separate class, and a lot of other stuff too.

src/main/java/org/openjdk/shenandoah/ShenandoahVisualizer.java line 667:

> 665:     }
> 666: 
> 667:     public static class RenderLive extends Render {

The render classes can go into their own files.

src/main/java/org/openjdk/shenandoah/Snapshot.java line 54:

> 52:         }
> 53: 
> 54:         //decodes for 3 bits older versions of shenandoah collector

Please add space after //

src/main/java/org/openjdk/shenandoah/Stopwatch.java line 2:

> 1: /*
> 2:  * ====

Bad copyright notice, again

src/main/java/org/openjdk/shenandoah/ToolbarPanel.java line 2:

> 1: /*
> 2:  * ====

And again

-------------

Changes requested by rkennke (Reviewer).

PR: https://git.openjdk.org/shenandoah-visualizer/pull/1