RFR: JMC-6555 Convert JOverflow plugin to SWT

[Jie Kang]

On Wed, Aug 21, 2019 at 10:41 AM Arvin Kangcheng Xu <kxu at redhat.com> wrote:
>
> Hello all,
>
> This change aims to port JOverflow from using JavaFX to SWT/JFace, so
> the extra dependencies could be dropped. Additionally, this change
> makes JMC-6343 is no longer valid.
>
> This port (re)implements all features available in the original
> version, and UI design is kept as close as possible. Please let me
> know your thoughts. Thank you very much!
>
> Also, thanks Jie for creating this webrev for me!
>
> Webrev:
> https://cr.openjdk.java.net/~jkang/joverflow-swt/webrev.01/
> Bug:
> https://bugs.openjdk.java.net/browse/JMC-6555

Hey Arvin,

Overall nice work! Visually and functionally (minor manual testing) it
looks to be working quite nicely. I have some comments and nits below,
separated in two sections. The nits in the second section can be
addressed in follow-up JIRA issues + patches.

# Section One

##
+import org.eclipse.swt.widgets.*;

There are a number of imports of x.y.* packages. Are all the classes
from these required? If not I would use individual imports. This makes
it easier to see exactly what package a class comes form.

##
With the removal of openjfx, it looks like org.openjdk.jmc.javafx.osgi
is no longer used. Can this be removed as well? As well, can the
javafx dependencies defined in the p2 repository and set in the target
platform files also be removed?

##
There are various instances of whitespace being included for newlines.
Please try to remove these, probably most easily done with an
auto-formatting tool. For what it's worth, in my terminal, invocations
of `hg diff` shows these as big blocks of red. Also it seems some
files use tabs and others use spaces. I think all should be switched
to tabs for this project.

##
+++ b/application/org.openjdk.jmc.joverflow.ui/src/main/java/org/openjdk/jmc/joverflow/ui/JOverflowUi.java
+    private boolean mIsUpdatingModel;
[...]
+    private void updateModel() {
+        if (mIsUpdatingModel) {
+            // skip updating if busy
+            return;
+        }
+
+        mIsUpdatingModel = true;
[...]
+        mIsUpdatingModel = false;
+    }
+
+    void reset() {
+        mIsUpdatingModel = true;
[...]
+        mIsUpdatingModel = false;
[...]

The thread safety of this construct is concerning. Is it expected to
only be called by a single thread? If yes, I'd appreciate a comment
specifying that it is not thread-safe, but it is okay because no more
than one thread calls this. If no, then I think a lock would be more
appropriate than a boolean.

Also there is a guard in updateModel() but no guard in reset(); is
that intentional? Can things go awry if reset occurs while updateModel
is executing? I guess these kinds of questions need only be considered
if there is multi-threaded access...

##
+++ b/application/org.openjdk.jmc.joverflow.ui/src/main/java/org/openjdk/jmc/joverflow/ui/viewers/BaseViewer.java
[...]
+    public void setHeapSize(long size) {
+        mHeapSize = size;
+    }
+
+    public long getHeapSize() {
+        return mHeapSize;
+    }

I don't see any usages of getHeapSize or of mHeapSize within this
class. Can they be removed?

##
+++ b/application/org.openjdk.jmc.joverflow.ui/src/main/java/org/openjdk/jmc/joverflow/ui/viewers/MemoryStatisticsTab
leViewer.java
+class MemoryStatisticsTableViewer extends TableViewer {
+
+       private long mHeapSize;
[...]
+       MemoryStatisticsTableViewer(Composite parent, int style,
Function<MemoryStatisticsItem, Color> colorProvider) {
[...]
+               createTableColumnViewer("Memory KB",
+                               model -> String.format("%d (%d%%)",
model.getMemory() / 1024, model.getMemory() * 100 / mHeapSize),
[...]
+       void setHeapSize(long size) {
+               mHeapSize = size;
+       }

Is it possible mHeapSize is *not* set via the package private function
before being used in the label provider created by the constructor? I
might initialize it to 1 just to prevent a possible NPE.


# Section 2: Can be addressed in follow-up JIRA issues + patches, or
immediately if easily done

##
The values displayed are no longer comma separated. For example
1234567 instead of 1,234,567. I think comma-separation increases
readability. If possible could this be added?

##
For the colour selection of arcs, some colours blend quite closely
with each other. While we're here it would be nice to only use
contrasting colours (rainbow).

##
The selection made in the Ancestor Referrer table persists though the
table contents change after selection. In the jfx version, the table
is deselected after. To reproduce, use the mouse to select a row in
the table. After selection, the table contents are changed but the row
(which has different content) remains selected. I think it's okay to
deselect after, or better yet, re-select the correct row.

##
When loading the same hprof file into JOverflow, some values are off
by one between the jfx and swt versions. Is it possibly due to
different rounding in the ui? It looks like the core api for joverflow
is unchanged. If so, this can probably be ignored. I saw this for
values in the Ancestor Referrer table.

##
+               // FIXME: Bug 165637 - [Viewers]
ArrayIndexOutOfBoundsException exception in ConcurrentTableUpdator
(used in DeferredContentProvider)
+        // https://bugs.eclipse.org/bugs/show_bug.cgi?id=165637
+               mContentProvider = new DeferredContentProvider((lhs, rhs) -> 0);
[...]
+                                       // FIXME: Bug 146799 - Blank
last table item on virtual table
+                       // https://bugs.eclipse.org/bugs/show_bug.cgi?id=146799

Ouch. Did you see these occur in practice? I guess it can be a
follow-up issue to resolve (probably by replacing with a different
implementation) but I'd be very reluctant to allow this to sit for any
length of time.

##
+++ b/application/org.openjdk.jmc.joverflow.ui/src/main/java/org/openjdk/jmc/joverflow/ui/JOverflowUi.java
[...]
+        mOverheadTypeViewer.setHeapSize(heapSize);
+        mReferrerViewer.setHeapSize(heapSize);
+        mClusterGroupViewer.setHeapSize(heapSize);
+        mAncestorViewer.setHeapSize(heapSize);
[...]
+        mReferrerViewer.allIncluded();
+        mClusterGroupViewer.allIncluded();
+        mAncestorViewer.allIncluded();
+        mOverheadTypeViewer.allIncluded();
+        for (ModelListener l : mModelListeners) {
+            l.allIncluded();
+        }
[...]
+        mOverheadTypeViewer.reset();
+        mReferrerViewer.reset();
+        mClusterGroupViewer.reset();
+        mAncestorViewer.reset();


These duplicate calls for the four sub-components feel sub-optimal,
especially in the allIncluded calls. Would it be appropriate to have
the for-loop construct for the listeners also be used to make the
calls to the sub-components as well? I.e. having the sub-components
added to the listener set?


Regards,

>
> Regards,
> Kangcheng