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