RFR: JMC-6555 Convert JOverflow plugin to SWT
Arvin Kangcheng Xu
kxu at redhat.com
Tue Aug 27 17:40:55 UTC 2019
Hello all,
The following is the updated webrev. Several issues mentioned in this
thread are addressed:
- wildcard imports were replaced with single class imports
- unnecessary white spaces were removed
- indentations were changed to using tabs instead of spaces
- removed mIsUpdatingModel guard
- removed getHeapSize and mHeapSize in BaseViewer
- declared setHeapSize in BaseViewer abstract
- initialized mHeapSize to 1 to avoid division by zero
- numbers are now rounded instead of truncated
- number displays are now comma-separated
- removed global jfx dependencies (javafx.osgi, p2 repo, target platforms)
- refactored sub-component calls
- used a more contrasting color palette for pie charts
Webrev:
https://cr.openjdk.java.net/~jkang/joverflow-swt/webrev.02/
Thanks to Jie for updating the webrev.
On Tue, 27 Aug 2019 at 09:40, Arvin Kangcheng Xu <kxu at redhat.com> wrote:
>
> On Tue, 27 Aug 2019 at 07:53, Mario Torre <neugens at redhat.com> wrote:
> >
> > On 26/08/2019 17:16, Arvin Kangcheng Xu wrote:
> > > I'm unsure if those calls are queued or executed right away. I
> > > remembered introducing this guard after I observed multiple updates
> > > caused by single user interaction. However, I took a further look at
> > > the code and experiment a bit. The guard is indeed unnecessary, at
> > > least for the current code.
> >
> > What I mean is that the event thread should be the one executeing this
> > method exclusively, so it cannot be that this code is accessed again
> > before the update is finished, it's only one thread.
> >
> > If you see this call executed multiple times it means it's being
> > accessed concurrently. I do not know enough of the Eclipse/SWT internal,
> > so I need to defer to Marcus or someone else for this review, however it
> > does seem to me that something is calling this method concurrently after
> > all, we should see what code does it and possibly fix it, because I
> > doubt this is correct.
>
> There is no new thread created once the heap model is loaded, so it cannot
> be accessed concurrently. The reason it was called multiple times was that I
> had called setSelection on a viewer as part of the updateModel call hierarchy,
> which triggers the callback where it handles user clicking and eventually calls
> updateModel again. Calls to setSelection were removed later, but not this
> guard.
>
> At least that's what I thought what happened. There is a high
> likelihood that I'm
> wrong. I do agree the code needs extra reviews.
>
> > Cheers,
> > Mario
> >
> > --
> > Mario Torre
> > Associate Manager, Software Engineering
> > Red Hat GmbH <https://www.redhat.com>
> > 9704 A60C B4BE A8B8 0F30 9205 5D7E 4952 3F65 7898
More information about the jmc-dev
mailing list