RFR: JMC-6555 Convert JOverflow plugin to SWT

[Mario Torre]

On Mon, Aug 26, 2019 at 3:16 PM Jie Kang <jkang at redhat.com> wrote:

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

I'm not an expert of the Eclipse threading model, but I would be very
surprised if the UI was updated outside the rendering thread, and afaik SWT
does not allow access from multiple thread on its component hierarchy, so I
would rather add a comment if this code was meant to be accessed by
multiple threads since that would be the exception.

Cheers,
Mario



> ##
> +++
> 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
>


-- 
Mario Torre
Associate Manager, Software Engineering
Red Hat GmbH <https://www.redhat.com>
9704 A60C B4BE A8B8 0F30  9205 5D7E 4952 3F65 7898