RFR: JMC-6555 Convert JOverflow plugin to SWT
Arvin Kangcheng Xu
kxu at redhat.com
Mon Aug 26 20:56:52 UTC 2019
On Mon, 26 Aug 2019 at 15:32, Jie Kang <jkang at redhat.com> wrote:
>
> 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.
No problem. Will remove in my next updated patch.
> > >
> > > > ##
> > > > 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.
It seems like JFace would not handle these selection updates if using
ILazyContentProvider (which DeferredContentProvider implements). Could
this potentially be a part of the follow-up issue where we replace
DeferredContentProvider with something else?
> > >
> > > > ##
> > > > 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
>
I see. I'll leave it unchanged. Thank you for correcting me.
> > >
> > > > ##
> > > > + // 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.
>
Aha I see what you mean now. mOverheadTypeViewer is speical and
includes all ObjectCluster regardless if they're filtered by
mOverheadTypeViewer (unlike all other viewers). That's why the
previous JOverflowFxUi implementation only included three of the
viewers. I'll revert it to something like before.
>
> 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