Bugzilla – Full Text Bug Listing
| Summary: | refreshChildren() optimization. | ||
|---|---|---|---|
| Product: | [Tools] GEF | Reporter: | Sergey Sharov <zefick> |
| Component: | GEF-Legacy GEF (MVC) | Assignee: | gef-inbox <gef-inbox> |
| Status: | RESOLVED WONTFIX | QA Contact: | |
| Severity: | normal | ||
| Priority: | P3 | CC: | Ed.Merks, jakub.mozgawa, nyssen |
| Version: | 3.8 | ||
| Target Milestone: | --- | ||
| Hardware: | PC | ||
| OS: | Windows 7 | ||
| Whiteboard: | |||
The List returned in getModelChildren() is an Arraylist (which is a random-access list), which is created inside addChild(). As get() will thus be performed in O(1) the time complexity should be the same, actually the index based access should be slightly faster compared to the use of an iterator. Resolving as WONTFIX. I wonder, does the list grow during the iteration?
If not, the following would be faster:
for (i = 0, size = modelObjects.size(); i < size; i++) {
If it does grown an iterator would break, so I assume the list has fixed size during the loop.
(In reply to Ed Merks from comment #2) > I wonder, does the list grow during the iteration? > > If not, the following would be faster: > > for (i = 0, size = modelObjects.size(); i < size; i++) { > > If it does grown an iterator would break, so I assume the list has fixed > size during the loop. The size remains the same, you are right. Actually I would have expected this to be optimized by the compiler... No, the compiler doesn't know the semantics of size(), and here you have a List, which could have any implementation so it's not so likely to be in-lined either. So the only time I ever write code like this is if I can't use a for-each loop because of concurrent modification exceptions, so I know the size grows and so I need to test it properly.
In general the compiler is not good at figuring out loop invariants so anything (and everything) that can be done outside each iteration is best done outside the iteration.
So you should never see or write code like this:
final X x = ...;
for (....)
{
x.getY().addAll(z);
}
because the compiler can't know if x.getY() will return something different each time but you, being aware of the API semantic can know that's not the case.
|
The documentation of AbstractEditPart.refreshChildren() method says that this method requires linear time to complete. But it's not as if getModelChildren() returns not a random-access list. In this case the method has quadratic complexity. The main cycle of the method starts with next lines: List modelObjects = getModelChildren(); for (i = 0; i < modelObjects.size(); i++) { model = modelObjects.get(i); I suggest to use an iterator and replace these lines to the following: Iterator iter = getModelChildren().iterator(); for (i = 0; iter.hasNext(); i++) { model = iter.next();