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

Thu Mar 30 11:26:56 UTC 2023

On Tue, 28 Mar 2023 19:49:19 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 51 additional commits since the last revision:
> 
>  - Fix comment formatting
>  - Update copyrights
>  - WIP: Fix issues for live mode
>  - WIP: Use a fixed size buffer for events
>  - Show initial logFile argument in toolbar
>  - Do not load log files on event loop thread
>  - Factor visualizer frame setup out of main function
>  - Fix weird 3-space indentation
>  - Layout improvements
>  - WIP: Fix warnings
>  - ... and 41 more: https://git.openjdk.org/shenandoah-visualizer/compare/dadfaff6...06abcd90

I did a cursory review for this, and I am thinking what to do next. There are many small things need to be done to improve this code, and going back-and-forth with reviews would probably be inefficient.

There are few ways we can proceed:
  1. I keep putting all the comments here, and you work on them.
  2. We push the PR in current form, knowing it works for you. Then we massage the code in subsequent PRs.
  3. I do the series of cleanups, give you the patch for this PR, you test it works, then we push the PR in new form.

Tell me which one do you prefer?

.github/workflows/pre-integration.yml line 13:

> 11:     strategy:
> 12:       matrix:
> 13:         java: [8, 11, 15, 16-ea]

While you are at it, you might want to change `java:` to `[8, 11, 17, 20, 21-ea]`

pom.xml line 18:

> 16:     <properties>
> 17:         <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
> 18:         <javac.target>19</javac.target>

Why do we need the target = 19? This would break GHA tests that run with JDK 8, I think. I would prefer to keep the baseline at JDK 8, if possible. If you need something more recent, then the latest LTS would be better, so target 17? You'll need to drop some JDK configs from GHA testing though.

src/main/java/org/openjdk/shenandoah/CircularBuffer.java line 31:

> 29: public class CircularBuffer<T> {
> 30: 
> 31:     public static final int DEFAULT_SIZE = 8;

Why `public`? Do you want it accessible in tests? You can drop `public` then, and rely on package-private visibility for this constant.

src/main/java/org/openjdk/shenandoah/CircularBuffer.java line 79:

> 77: 
> 78:     public int size() {
> 79:         return Math.min(count, elements.length);

Why not just `return count`?

src/main/java/org/openjdk/shenandoah/Colors.java line 71:

> 69:     static final Color AGE_3 = Color.RED;
> 70:     static final Color AGE_4 = Color.BLUE;
> 71:     static final Color AGE_5 = Color.BLACK;

Are these used anywhere? You can just inline them into `AGE_COLORS`?

src/main/java/org/openjdk/shenandoah/DataConnector.java line 73:

> 71: 
> 72:     enum State {
> 73:         Searching, Connecting, Connected, Disconnecting, Disconnected

Style: enum constants should be capitalized.

src/main/java/org/openjdk/shenandoah/DataConnector.java line 95:

> 93: 
> 94:     @Override
> 95:     public void run() {

This should be a nested class, something like `DataConnector.ConnectTask`. Saves you from some foreign code to accidentally put `this` into another executor.

src/main/java/org/openjdk/shenandoah/DataConnector.java line 104:

> 102:                     subscribeToGarbageCollectorNotifications(server);
> 103:                     monitoredVmConsumer.accept(vm);
> 104:                     shouldRun = false;

`shouldRun` looks redundant,  can just `break` out of the `while(true)` loop here.

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

> 47:             String metaDataLine = br.readLine(); // timestamp status numRegions regionSize
> 48: 
> 49:             while (metaDataLine != null && metaDataLine.trim().length() > 0) {

This is better written as:

Suggestion:

            // Metadata line: timestamp status numRegions regionSize
            String metaDataLine;
            while ((metaDataLine = br.readLine()) != null) {
                if (metaDataLine.trim().isEmpty()) continue;

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

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

Can be package-private then.

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

PR Review: https://git.openjdk.org/shenandoah-visualizer/pull/1#pullrequestreview-1364770965
PR Review Comment: https://git.openjdk.org/shenandoah-visualizer/pull/1#discussion_r1153032651
PR Review Comment: https://git.openjdk.org/shenandoah-visualizer/pull/1#discussion_r1153034482
PR Review Comment: https://git.openjdk.org/shenandoah-visualizer/pull/1#discussion_r1153036929
PR Review Comment: https://git.openjdk.org/shenandoah-visualizer/pull/1#discussion_r1153051320
PR Review Comment: https://git.openjdk.org/shenandoah-visualizer/pull/1#discussion_r1153041557
PR Review Comment: https://git.openjdk.org/shenandoah-visualizer/pull/1#discussion_r1153042478
PR Review Comment: https://git.openjdk.org/shenandoah-visualizer/pull/1#discussion_r1153044701
PR Review Comment: https://git.openjdk.org/shenandoah-visualizer/pull/1#discussion_r1153046373
PR Review Comment: https://git.openjdk.org/shenandoah-visualizer/pull/1#discussion_r1153058909
PR Review Comment: https://git.openjdk.org/shenandoah-visualizer/pull/1#discussion_r1153059852