Sv: RFR: JMC-6555 Convert JOverflow plugin to SWT
Marcus Hirt
marcus at hirt.se
Wed Aug 28 11:24:23 UTC 2019
Hi all,
Regarding the removal of the JavaFX bundles, I'll simply note that Oracle has a JavaFX based plug-in for Coherence. Removing the third-party JavaFX bundling from the p2 site would make it much harder to have JavaFX based plug-ins in most versions of JMC (except Oracle's, who would likely override stuff to retain this capability). This is not necessarily a bad thing - harmonizing around SWT (and Swing for some drawing on the AWT_SWT bridge) would simplify many things. Other things would OTOH become much harder. For example, I once prototyped a rather nice 3D heap region and occupancy viewer using JavaFX.
Kind regards,
Marcus
-----Ursprungligt meddelande-----
Från: jmc-dev <jmc-dev-bounces at openjdk.java.net> För Jie Kang
Skickat: den 26 augusti 2019 21:32
Till: Arvin Kangcheng Xu <kxu at redhat.com>
Kopia: jmc-dev at openjdk.java.net
Ämne: Re: RFR: JMC-6555 Convert JOverflow plugin to SWT
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/o
> > > +++ penjdk/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/o
> > > +++ penjdk/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/o
> > > +++ penjdk/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/o
> > > +++ penjdk/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