Use SwingWorker to read GCModel in a background thread

[hbswn]

In the current 1.33 implementation the Swing event dispatcher thread is blocked during loading the GCModel. It means that you cannot select anything in the menu, you cannot even quit the application.

I've created a fix in https://github.com/hbswn/GCViewer
It's not production ready, but for developers it's nice to play with.

I added:

• GCModelLoader extends SwingWorker<GCModel, Object>: loads GCModel and sends progress update events
• MonitoredBufferedInputStream: a simple (but not complete) way to monitor progress
• ShutdownHandler: make sure the threads are interrupted, when the application is stopped. (but maybe ExecutorService.shutdown* methods do a better job)
• A parser Logger per URL: otherwise concurrent logging causes some weird behaviour. (info for other files being logged in the error window)

Some issues:

• The progress bar does not look very well but it works
• The progress code only uses the input file position. It should monitor the entire loading process including parsing
• DataReaderFacade is now a singleton. I needed a single "DataReader" thread group and ThreadFactory. Maybe a better solution exists.
• I would prefer to create GCModel before the DataReader is started. Then we can add some properties to monitor the loading process (e.g. file size, date and the current position). GCModel can be passed to the reader and returned, when it's done. Also the parser logger can be created in GCModel and with GCModel's hashCode in its name.
• If loading a file/URL failed, then the item is not removed from the Window list
• GCModelLoader should be canceled if ChartPanelView is closed

Let me know, what you think.
(maybe I should create a Google+ account)

[hbswn]

I should add, that we must code thread-safe in the code used by readModel.
I've checked some code and found it all OK, since it uses (reader)Object-local variables.
Each reader has its own data, so it won't affect other readers (concurrently running in other threads).

[chewiebug]

Hi Hans,

I like your idea of using threads to load the files! The blocking of the
gui in the current implementation is not really nice.

I am not much into multi threading recently, so my questions might show
just that ;-).

• Is the shutdown handler really needed? Couldn't demon threads be
  used? As far as I know, they would be stopped, when all user threads
  are stopped.
• Why is the thread group in the DataReaderFacade needed?

Looking into the code, I have a small wish list:

• There are quite a few inner classes. I think I would prefer most of
  these to be classes in their own java files.
• I think, I would like to see the progress bar in it's own window
  separate from the ChartPanelView. Those should only open, if loading
  of a log file completed successfully. This might be quite some
  refactoring however.

What do you think?

Best regards, Jörg

PS: Why the Google+ account?

[hbswn]

I do not have much GUI/Swing experience, so I guess we complement each other.

• shutdown-handler: maybe other solutions work. I already mentioned the ExecutorService.shutdown* methods, which probably have their own shutdown handler.
• It's easier to control all threads through a ThreadGroup. All user threads have to be interrupted/stopped before the jvm can exit. Debugging is also much easier if all user threads are grouped logically.
  On user vs. daemon threads: for this kind of temporary application-level work user threads should be used. Daemon threads are more for jvm-level maintenance work. Main reason for not using daemon threads for normal work, is that the jvm will not wait for anything to complete. i.e. it can be stopped in the middle of an IO operation.
• Inner classes: agreed. My only reason for this implementation was, that I did not want to make too big changes, yet. I also prefer separate classes unless they have a very close relation to another class.
• progress bar: agreed. I only wanted to show how it works. I'm not that good in Swing work, yet.

Google+ account? Maybe better for discussions.

Maybe, with some more refactoring, the GCModelLoader can be the creator of the ChartPanelView. i.e. Launch the ChartPanelView from GCModelLoader.done(). Also saves some clean-up work if loading the model fails.

[chewiebug]

Hi Hans,

Thank you for the explanations! Now I understand better, why the
thread-grouping is needed.

I believe, that refactoring the inner classes into their own classes
will make the code easier to understand. I am happy that you agree. It's
ok to begin small and change later to a better solution.

It sounds like a good idea to me, to launch the ChartPanelView from
GCModelLoader.done(). Or at least fire an event there and create it
somewhere else. Then you already know, that you want to create it. Did
you see that the warnings logged are suppressed in the current
implementation?

Do you know about the mailing list
https://github.com/chewiebug/GCViewer/wiki/Mailinglists for GCViewer?
May be, that would suit you better?

Best regards, Jörg

[hbswn]

I've removed the ThreadGroup, ThreadFactory and ShutdownHandler and it still works fine. SwingWorker.execute() does a good job installing using its own shutdown handler.

I do not have time, yet, to extract the GCModelLoader from ChartPanelView.

[hbswn]

Hi Jörg,

I've missed your comment, while I was busy with my next build, commit, push and the comment about it.

Warnings by the parser, you mean?
I think, what I suggested in my first comment, a logger in GCModel (per instance) would solve that nicely. Then all logging related to model loading can be done with a logger that's bound to the model (and hence to the file being loaded and parsed).

On the mailing list: I know and I already have the RSS feed in my RSS reader (Liferea).

Best regards,

Hans

[hbswn]

I broke the export function, because I forgot to setUrl on the GCModel.
Fixed it and added a test for this to TestDataReaderFacade.

[chewiebug]

Hi Hans,

You did quite some work again, thank you!

Extraction of GCModelLoader from ChartPanelView

Might be difficult, because I think it is in an area that is not well
modularized. If you have time - cool, otherwise I'll give it a try later.

Warnings by the parser not shown. Suggestion to add them to the GCModel.

I fear, that I didn't understand your suggestion when I first read it.
Now I have understood it. Why not.

I'll need a few days until I'll be able to look into your code and test it.

Best regards, Jörg

[hbswn]

The problem I had with the original static parser, was that messages for all concurrently loading files were displayed in all textAreas.

[chewiebug]

Ok, I see. That's part of what wasn't thread safe.

[hbswn]

My first attempt to load the model before creating a ChartPanelView:
(from GCViewer 1.33)

Added to GCDocument:

   List<GCModelLoader> modelLoaders;

   public void add(final URL url) {     
        final GCModelLoader modelLoader = new GCModelLoader(this, url);
        modelLoaders.add(modelLoader);
        modelLoader.execute();
    }

    public void gcModelLoaderDone(final GCModelLoader modelLoader, final GCModel model) {
        modelLoaders.remove(modelLoader);
        if (model != null) {
            createChartPanelView(model);
            gcViewer.repaint();
            repaint();
        }
    }

    protected void createChartPanelView(final GCModel model) {
        //In the ChartPanelView constructor I've replaced URL with GCModel (which contains URL).
        final ChartPanelView chartPanelView = new ChartPanelView(this, model);
        chartPanelViews.add(chartPanelView);
        .....
        //... remainder of the original add(URL) method ...
    }

In other words:

• addUrl creates a GCModelLoader for the URL and adds it to modelLoaders.
  Then it starts the SwingWorker ("execute()" )
• From GCModelLoader.done() it calls back to GCDocument.gcModelLoaderDone() and:
  • removes itself from "modelLoaders"
  • on success: creates a ChartPanelView using the model and repaints

This works fine, but I do not have any error handling nor any progress indicator.
If all urls fail to load, you're left with an empty GCDocument window.
Since all loading is done asynchronously, the "throws DataReaderException" of

    GCDocument.add(url) 
    GCDocument.reloadModels(boolean) // called from Refresh.actionPerformed()

do not make sense, anymore.

And I do not know where/how to implement reloadModel, yet. The easiest would be to just create a new ChartPanelView with the new model and replace the old one. But maybe it's better to reload the data into the existing ChartPanelView.

[hbswn]

Hmm, maybe modelLoaders is not needed at all. I added it, because I expected it needed to be maintained, somehow. Well, maybe when GCDocument is closed.

[hbswn]

I've added a GCModelLoaderView, which contains:

• a JProgressBar
• the TextAreaLogHandler + JTextArea
• a JLabel to display an error message if any

It shows either the JProgressBar or the error message, depending on the result.

Still some issues:

• reloading the model does not work
• the GCModelLoaderView cannot be removed if there are errors.
• only errors on opening the stream are currently reported.
• relayout() should better not be called too often: some queueing through Swing would be better. (update events that can be ignored if not relevant anymore)

I'd prefer to pass the new GCResource to the DataReader implementation, but then a lot of code has to be modified.
The advantages:

• GCResource.logger can be used everywhere where the GCResource is used.
• GCResource can get additional properties: e.g. the ranges of issue min / max interval for full gc #14 (min / max interval for full gc)
• progress can be monitored by GCResource instead of the ugly callback in DataReaderFacade.readModel()

[chewiebug]

Hi Hans,

Looks good! Concerning "relayout()" you sure are right - that is something I am not much happy about.

I have given some thoughts to the issues you still have. Maybe a separate controller class responsible for loading and a window derived from JDialog displaying all progress could help there. Something like this:

I think, this should help with reloading the model (tell the loader to load GCModel for given URL / GCResource and replace model inside ChartPanelView). I hope it also helps with error handling. And it separates loading logic even more from the gui logic.

What about adding another tab to ChartPanelView to display parser problems in cases where loading was possible? If Loading is not possible at all, GCViewerGui should show a dialog similar to todays dialog.

Does your GCResource replace your earlier idea about adding the parser error messages to GCModel? Or is it an additional helper. I'd find storing the error messages in GCModel a good idea. Then displaying the errors in a separate tab is no problem.

I am about to write the GCModelLoaderController, but I don't know, when I'll finish it. If you are faster changing things, just go ahead.

Regards, Jörg

[hbswn]

Indeed, first I had the idea of creating a GCModel in the GCModelLoader, pass it to the reader and get it back.
I replaced that with creating a GCResource, pass it to the reader (instead of a URL) and getting it back in the GCModel. i.e. GCResource = the minimum item to:

• identify the resource (same function as the URL)
• have the Logger attached to it
• possibly be extended with extra properties that identify it (like a range restriction)
• contain progress data

[hbswn]

A tab in ChartPanelView might be a good solution for reloading.

In my latest implementation ChartPanelView is only created after loading+parsing has completed successfully.
I think that's good. Have you tried loading more than one file at a time?
I'm not that good in GUI work, but I thought a standard JDialog would make it more complicated to handle multiple (concurrent) GCModel loads.
And I do not like to get many dialog boxes at (almost) the same time.

[chewiebug]

GCResource seems to be a sensible unit. Thank you for explaining the details.

Ah, I see! You open a GCDocument and show the GCModelLoaderView inside. Upon successful completion you replace it with a ChartPanelView. My Idea was close to yours: I wanted to open the GCDocument only after completion of parsing. To display the progress, I wanted to introduce one JDialog (non modal window; always on top) showing all models currently being loaded in the background. As soon as one would be finished, the appropriate GCDocument would be opened or updated (responsibility of GCViewerGui; GCModelLoaderController just informs GCViewerGui about completion).

My idea to handle complete failures in its own popup window (as today) is indeed not a good idea. I don't like many dialog boxes opening swiftly. I like your approach better using a GCDocument to display the parser messages.

Opening several files at once works nice with your implementation!

I think, I'll have to try a bit more with my Controller class and maybe the JDialog.

[chewiebug]

Hi Hans

I have tried to add my controller this weekend. I ran into a problem however: Often the chart is not drawn after loading of the file (mostly if the file is passed as a command line parameter, but also in other cases). Do you also experience this behaviour? I tested with your latest commit. The second last commit does not have this symptom.

Regards, Jörg

[hbswn]

Hi Jörg,

Maybe yours is a different issue, but when I experienced something similar, I first found that it also failed in the 1.33 version and then I found that my settings were changed: all checkboxes in the "View" menu were blank, so I only got the chart without any data in it. No idea, what caused this.

Hans

[chewiebug]

Hi Hans,

It is the checkboxes unchecked in my case as well. Not as robust as I'd like it to be :-/.

I can't reproduce it with 1.33. Can you give me the steps on how to reproduce it?

Regards, Jörg

[hbswn]

I did not create the issue with 1.33. Only after I had it with my latest build, I tried 1.33 and it also happened.
So I suspected some configuration problem and then found the cleared checkboxes.

[hbswn]

Apparently somehow the JCheckBoxMenuItem.state changes to false for all chart checkboxes in the View menu when loading a chart with my version.

[hbswn]

From GCViewerGui.internalFrameActivated():

            menuItemFullGCLines.setState(getSelectedGCDocument().getModelChart().isShowFullGCLines());
            menuItemIncGCLines.setState(getSelectedGCDocument().getModelChart().isShowIncGCLines());
            .......
            menuItemAntiAlias.setSelected(getSelectedGCDocument().getModelChart().isAntiAlias());

From ModelChartImpl:

    public boolean isShowFullGCLines() {
        return fullGCLineRenderer.isVisible();
    }

So GCViewerGui.internalFrameActivated() is called, while the chart (and fullGCLineRenderer) is not visible, yet.

[chewiebug]

Hi Hans,

Thank you for tracking this down! I've come to the same conclusion.

By the way: I don't suspect you to have introduced this bug. I believe that a bad decision I made earlier led to this behaviour. I didn't program it to be robust. But I still can't reproduce it using 1.33. If you can I'd be grateful to know how. Because then I'd like to fix this as soon as possible for 1.33.

Regards, Jörg

[ecki]

I think I have seen the same problem with an older Version before (very sporadic). So I guess its a race.

Bernd

Am 19.12.2013 um 22:11 schrieb chewiebug notifications@github.com:

Hi Hans,

Thank you for tracking this down! I've come to the same conclusion.

By the way: I don't suspect you to have introduced this bug. I believe that a bad decision I made earlier led to this behaviour. I didn't program it to be robust. But I still can't reproduce it using 1.33. If you can I'd be grateful to know how. Because then I'd like to fix this as soon as possible for 1.33.

Regards, Jörg

—
Reply to this email directly or view it on GitHub.

[chewiebug]

@ecki thank you for confirming the bug with the white chart.

I have pushed a fix to #87-swingworker branch. The white charts should not occur any more.
@hbswn If you continue to work on this feature, could you please merge this commit into your branch (I'll send you a pull request)?

As a next step I'll try to refactor GCViewer towards the MVC pattern. Currently the views often are also the controller and sometimes contain model elements. I suspect, this makes the addition of some features here very difficult (like reloading of the model).

Regards, Jörg

[chewiebug]

Hi Hans,

Sorry for taking so long! I have had little time in the last few months and found some issue with my refactoring that took me quite some time to get fixed. I have pushed another branch to my repository (#87-swingworker-fixed) where I fixed that issue I had with the refactoring in the #87-swingworker branch (which I now deleted).

I believe, I have been able to go a step towards the MVC pattern in the gui layer, allthough not the whole way yet. I think, I have add most of the suggestions you had not added yet.

The most obvious feature missing yet is the possibility to interrupt a thread loading a gc file, if it takes too long.

Best regards,
Jörg

[chewiebug]

I have renamed the branch to feature/#87/swingworker.

[chewiebug]

I have now merged the branch into "develop" and close this issue.