MigLayout 5.0-SNAPSHOT performs slowly when calculating layout for a lot of nodes in JavaFX 8

[GoogleCodeExporter]

Since it resolves issue #6, I migrated my code to MigLayout 5.0-SNAPSHOT.

Running the full application, I took a severe performance hit:
When adding a good number of nodes, part of them stacked MigLayouts to a scene, 
my application spend most of its time calculation the new layout, becoming 
inresponsive for a good while.

My profiler indicates that this is due to the hashcode calculation, where you 
concatenate a number of Strings.

I have attached the profiling results. 

If you need one, I will try to come up with a running example of this issue.

Original issue reported on code.google.com by ursreu...@gmail.com on 12 Apr 2014 at 12:06

Attachments:

• Call_Tree.html
• Hot_Spots.html

[GoogleCodeExporter]

To view the HTML files, please download them.

Original comment by ursreu...@gmail.com on 12 Apr 2014 at 12:11

[GoogleCodeExporter]

The issue was probably introduced in b36b23e5a4d780eb44d68c8299b870f89371f4b4, 
where you added the validation calls to size calculations.

Original comment by ursreu...@gmail.com on 12 Apr 2014 at 12:17

[GoogleCodeExporter]

Since calculateHashcode(Node) is only used to check for invalidation, it should 
be safe to change the implementation away from Strings.
Since the hashCode() implementation used by String is nothing special - it just 
iterates over the characters and multiplies their values with a prime - you 
could do something similar here without taking the detour.

(After all this analysis, it seems most simple to send you a pull request, but 
I can't figure out how to do it on Google Code.)

Original comment by ursreu...@gmail.com on 12 Apr 2014 at 12:48

[GoogleCodeExporter]

True, using a string made it easy to debug, but that can be done better. This 
is the way this has been done from day 1, so it is not some kind of regression. 
The idea behind MigLayout is that you do not need to nest multiple MigPanes. 

Never the less, it is a performance hit, so I'll take a look tonight. 

Original comment by tbeer...@gmail.com on 12 Apr 2014 at 1:49

[GoogleCodeExporter]

Not a regression, agreed. I didn't mean to imply it was one, sorry if I came 
across that way.

Re: Not nesting multiple MigPanes
I know the theory, but I never figured out how to do it in a modular 
application with decentralized control over layout. This doesn't seem the 
proper place to discuss it, so I have taken it back to StackOverflow:
http://stackoverflow.com/questions/23032051/how-to-create-a-modular-ui-using-mig
layout-without-nesting-multiple-panels

Original comment by ursreu...@gmail.com on 12 Apr 2014 at 3:04

[GoogleCodeExporter]

I've replaced the hashcode method with a more standard implementation. But it 
still is a computation intensive approach. Could you check if not using a 
string reduces the performance hit?

Original comment by tbeer...@gmail.com on 12 Apr 2014 at 6:33

[GoogleCodeExporter]

Anything to watch out for during the build?
Just clone the repo, do "mvn install" and change my build file accordingly? 

Original comment by ursreu...@gmail.com on 12 Apr 2014 at 7:44

[GoogleCodeExporter]

Yes, or simply load the miglayout-core and miglayout-javafx as projects in 
Eclipse (or other IDE)

Original comment by tbeer...@gmail.com on 12 Apr 2014 at 7:49

[GoogleCodeExporter]

The addition-scenario performs within reasonable parameters now.
I have attached the updated profiling results for the record. I didn't write 
down my order of clicks for the first run, so they aren't as comparable as I'd 
like them to be.

However, the issue doesn't seem fully resolved yet. It appears that the same 
calculation run is triggered whenever I interact with a Node, which means that 
every click now comes with the same cost in validation as does re-building the 
entire screen.

I don't know if I'll be able to re-create this as a testing scenario outside of 
my application, but I could have a look at improving the performance myself if 
you could tell me what the intent behind the added validation calls is.
(Or you just tell me: "Don't stack the MigPanes." I can live with that.)

Original comment by ursreu...@gmail.com on 13 Apr 2014 at 6:06

Attachments:

• Hot_Spots.html

[GoogleCodeExporter]

Well, it is good to hear the there is at least a performance improvement, but I 
kinda figured that it would not solved the problem, because it still does a lot 
of calculating. 

Please be aware that I only wrote the JavaFX wrapper, many years ago already, 
and it was based on the Swing version. So some pieces of code are there just 
because they were in the Swing version. This is also the case for the hashcode 
/ invalidate logic; it appears to be a performance optimization to detect if 
any of the nodes have changed and a new layout should be calculated. 
Unfortunately the data I use to determine if a node has changed, are not all 
JavaFX Properties, so I cannot listen to changes.

Mikael told me once that migLayout was not intended to be embedded, so I can 
only relay that message.

Original comment by tbeer...@gmail.com on 13 Apr 2014 at 7:40

[GoogleCodeExporter]

I see.
However, maybe you still do remember why you added the various calls to 
    validateMigLayoutGrid();
to the compute**Height()/compute**Width() methods, as it was only last 
December[1].
Armed with that knowledge, I could search for a solution without interfering 
with your work.

For reference, I have attached another profiling run, this time of the scenario 
that I describe above, my clicking a single button over and over again, each 
time triggering a validation.

[1] 
https://code.google.com/p/miglayout/source/diff?spec=svnb36b23e5a4d780eb44d68c82
99b870f89371f4b4&r=b36b23e5a4d780eb44d68c8299b870f89371f4b4&format=side&path=/ja
vafx/src/main/java/org/tbee/javafx/scene/layout/MigPane.java

Original comment by ursreu...@gmail.com on 13 Apr 2014 at 7:46

[GoogleCodeExporter]

What you could try, if you have the migpane-javafx source available, is 
commenting out some lines in the calculateHashcode method. I've tried to be 
thorough and placed making sure the layout was correct above performance, but 
you could try and comment out the min* max* and getLayoutBounds* lines of the 
calculation. See how it holds up in your application.

Original comment by tbeer...@gmail.com on 13 Apr 2014 at 7:51

[GoogleCodeExporter]

I dug up the emails from December; the additional validate calls were needed so 
MigPane knows how big the actual pane would like to be. The issue had to do 
with opening a scene without specifying the size, MigPane would then determine 
its prefered size and not respond to nodes being added after it was created. 
This is why the Pane's size methods need to do an invalidation. 

That said, clicking on a button should of course not trigger a validation run. 
But I cannot reproduce that behavior myself.

Original comment by tbeer...@gmail.com on 13 Apr 2014 at 9:19

[GoogleCodeExporter]

Thanks for the insight. I tried removing the additional calls, and found out 
that they were what fixed issue #6.

I will try to reproduce the button issue in a more isolated example.

Original comment by ursreu...@gmail.com on 13 Apr 2014 at 9:38

[GoogleCodeExporter]

That is indeed what I expected.

Original comment by tbeer...@gmail.com on 13 Apr 2014 at 10:22

[GoogleCodeExporter]

I didn't make much progress trying to isolate the rest of the issue - I have a 
panel with some 200 nested MigLayouts refresh in the blink of an eye.

There is probably something in my application code that interacts with JavaFX 
or MigLayout in an adverse way, triggering the frequent refresh.

I won't mind if you close this issue, I will open a new one when I have 
something more concrete.

Original comment by ursreu...@gmail.com on 13 Apr 2014 at 4:41

[GoogleCodeExporter]

Hi guys,

MigLayout should run a validation when the UI framework ask it to. I don't 
think a UI framework should ask for a re-layout run if the button is pressed. A 
validation should on the other hand be lightning fast anyway but I haven't 
checked the code for this in JavaFX so I can't say. But Tom seems to be on to 
of that. 

I also noted that I don't get emails when there are post in this forum which is 
why I hadn't seen this until now.

Original comment by mikael.g...@gmail.com on 13 Apr 2014 at 4:51

[GoogleCodeExporter]

I have now looked at the JavaFX MigPane code a little. I don't think you need 
the validation phase in JavaFX. You should just have to override 
requestLayout() in MigPane and set grid = null there. It seems that any control 
that 

The isNeedsLayout() property is also something that might be needed to be 
checked. I'm, not sure how this relates to requestLayout().

Basically you want to catch if any of the children to MigPane needs layout 
since that means that the cached sizes (in grid) are invalid. One or both of 
the above might be needed for this.

At least that's what I think after glancing on the JavaFX API for a few 
minutes. 

Original comment by mikael.g...@gmail.com on 13 Apr 2014 at 5:15

[GoogleCodeExporter]

Mikael, in your first paragraph, there is an incomplete thought:
"It seems that any control that" - that what?

Original comment by ursreu...@gmail.com on 13 Apr 2014 at 6:54

[GoogleCodeExporter]

I tried overriding requestLayout, but the initial results are not promissing. 
The layout is drawn more often than with the validation. And given that Urs 
tried 200 nested MigPanes without a hickup, there is no immediate reason to 
refactor.

Marking issue as "cannot reproduce" for now.

Original comment by tbeer...@gmail.com on 13 Apr 2014 at 8:01

• Changed state: Invalid

[GoogleCodeExporter]

ursreupke, sorry, it should just be removed. I continued in the next section 
instead.

Original comment by mikael.g...@gmail.com on 13 Apr 2014 at 8:47

[GoogleCodeExporter]

Tom,

Ok. Makes sense actually since requestLayout is always called when laying out, 
even if that started from a parent (as per you description).

I have now checked some more. It looks like you can just call .isNeedsLayout() 
on the child components during validation of the grid. If at least one returns 
true then recreate the Grid (or rather set it to null). 

You should also invalidate the grid if LC or AC are changed and possibly for 
other properties on the MigPane itself.

I use grid = null to denote invalidate it. I just recreate it just before I 
need it again. That means that several invalidations only leads to one create. 
It is creating the Grid that is the most expensive in MigLayout. I think you 
now recreate it every time a component is added or removed and possibly for 
other things as well. This can be a major performance hit if you add and remove 
components a lot. For instance you create it in the constructor even though 
adding a component will recreate it.

I might be wrong though, this is just, again, by glancing quickly at the code. 
:)

Original comment by mikael.g...@gmail.com on 13 Apr 2014 at 9:36