RFR: JMC-6555 Convert JOverflow plugin to SWT

Jie Kang jkang at redhat.com
Mon Aug 26 19:32:10 UTC 2019 

On Mon, Aug 26, 2019 at 2:37 PM Arvin Kangcheng Xu <kxu at redhat.com> wrote:
>
> On Mon, 26 Aug 2019 at 14:36, Arvin Kangcheng Xu <kxu at redhat.com> wrote:
> >
> > On Mon, 26 Aug 2019 at 09:15, Jie Kang <jkang at redhat.com> wrote:
> > >
> > > 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.
> >
> > I agree. All wildcard imports will be replaced with single class imports.
> >
> > > ##
> > > 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?
> >
> > I only removed jfx related dependencies within the joverflow component itself and left the global ones untouched. Are there other dependents preventing us from removing jfx completely? If no further discussions are needed, I'll be happy to remove those, too.

I think they can be removed safely and should be.

> >
> > > ##
> > > 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.
> >
> > I will adjust my auto-format setting and reformat.
> >
> > > ##
> > > +++ 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...
> >
> > This is addressed in another email thread. mIsUpdatingModel is to be removed.
> >
> > > ##
> > > +++ 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?
> >
> > Yes. getHeapSize and mHeapSize will be removed. setHeapSize will be declared abstract.
> >
> > > ##
> > > +++ 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.
> >
> > I'm not aware of a better solution at this moment, but I'll initialize it to 1 to avoid ArithmeticException.
> >
> > > # 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?
> >
> > This should be an easy fix. Comma-separation will be implemented in the next update.
> >
> > > ##
> > > 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).
> >
> > Right now the colours are random. There is no colour palette since there is no telling how many colours are needed. I'll study the original implementation and see how it was handled.
> >
> > > ##
> > > 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.
> >
> > This is interesting and I can't provide an explanation of why it is happening at this moment. I'll look into this afterwards.

My text explanation may have been confusing but yeah sounds good to
look at it later.

> >
> > > ##
> > > 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.
> >
> > Yes, it is a difference in rounding. When converting from B to KB, the jfx version rounds and the swt version truncates. I'll change it to rounding.
> >
> > Also, do you think it's worth to change "KB" to the more accurate "KiB"?

I'd would follow whatever is done elsewhere in the project. Not sure
what you mean by more accurate though; my understanding from [1] KB ==
KiB != kB

[1] https://en.wikipedia.org/wiki/Kibibyte

> >
> > > ##
> > > +               // 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.
> >
> > Yes. I've seen this occasionally. DeferredContentProvider looks quite fragile [0].
> > However, virtual tables with an ILazyContentProvider is a must for this application since there are just too many entries. In theory, we can implement our own ILazyContentProvider that also does filtering and sorting like DeferredContentProvider, but what will be quite some work which deserves a follow-up.
> >
> > > ##
> > > +++ 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?
> >
> > Correct me if I'm wrong. Are you suggesting something like this?
> >
> > List<ModelListener> tmp = new ArrayList<>(mModelListeners);
> > tmp.addAll(Arrays.asList(mReferrerViewer, mClusterGroupViewer, mAncestorViewer, mOverheadTypeViewer));
> > for (ModelListener l : tmp) {
> >     l.allIncluded();
> > }

Er; I mean to say that the viewers can add themselves, or be added to
the listener array that already exists and the listener could be
expanded to also signal reset and heap size changed. So in the
constructor you could run:

mModelListeners.addAll(Arrays.asList(mReferrerViewer,
mClusterGroupViewer, mAncestorViewer, mOverheadTypeViewer));

You can see something like this in the previous JOverflowFxUi, though
they chose (on purpose I suppose?) not to add one of the viewers. Not
sure if you need to follow them exactly though.


Regards,

> >
> > Regards,
> > Arvin
> >
> > [0] https://bugs.eclipse.org/bugs/showdependencytree.cgi?id=509006&hide_resolved=1
> > >
> > > Regards,
> > >
> > > >
> > > > Regards,
> > > > Kangcheng

More information about the jmc-dev mailing list