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

[William Kemper]

On Tue, 14 Feb 2023 11:08:53 GMT, Roman Kennke <rkennke at openjdk.org> wrote:

>> William Kemper has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Add notes for running with JDK17+
>
> 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?

This is an IDE directive to stop it from complaining about the unchecked cast. I can remove it.

> 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.

I'll see what I can do here.

> 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,

I will look into this. Agree it should be a separate change.

> 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.

It's debugging. I can have this use JUL?

> 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 ?

Agreed, I'll change this.

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

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