Bugzilla – Full Text Bug Listing
| Summary: | [xtext][ui][refactoring] performance problem in LiveShadowedResourceDescriptions | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | [Modeling] TMF | Reporter: | Knut Wannheden <knut.wannheden> | ||||||
| Component: | Xtext | Assignee: | Project Inbox <tmf.xtext-inbox> | ||||||
| Status: | CLOSED FIXED | QA Contact: | |||||||
| Severity: | normal | ||||||||
| Priority: | P3 | CC: | sebastian.zarnekow | ||||||
| Version: | 2.1.0 | Flags: | sebastian.zarnekow:
juno+
|
||||||
| Target Milestone: | M4 | ||||||||
| Hardware: | PC | ||||||||
| OS: | Mac OS X - Carbon (unsup.) | ||||||||
| Whiteboard: | |||||||||
| Attachments: |
|
||||||||
|
Description
Knut Wannheden
The way LiveShadowedResourceDescriptions has been implemented (i.e. by extending ResourceSetBasedResourceDescriptions) its getExportedObjects*() queries are very expensive to execute since the methods delegate to super.getExportedObjects*() at some point. This will in turn end up calling the same query on every IResourceDescription returned by LiveShadowedResourceDescriptions#getAllResourceDescriptions(). The latter also includes all resource descriptions from the global (delegate) IResourceDescriptions. So what should be a query against the "local" data of the LiveShadowedResourceDescriptions ends up including the global data in a very inefficient way. Instead I think the LiveShadowedResourceDescriptions should be more similar to DirtyStateAwareResourceDescriptions and have two delegates: a "localDescriptions" and a "globalDescriptions". This was fixed for 2.1. Could you please double check that for the released code base? This was tested against 2.1. I can see that parts of DirtyStateAwareResourceDescriptions were copied. But the local resources are maintained by LiveShadowedResourceDescriptions itself (throw the inheritance of ResourceSetBasedResourceDescriptions) which causes the problem as described. E.g. LiveShadowedResourceDescriptions#getExportedObjects(EClass, QualifiedName, boolean) -> AbstractCompoundSelectable#getExportedObjects(EClass, QualifiedName, boolean) -> ResourceSetBasedResourceDescriptions#getSelectables() -> LiveShadowedResourceDescriptions#getAllResourceDescriptions() -> globalDescriptions#getAllResourceDescriptions() I see - thanks for pointing this out. As you may have expected: a patch is most welcome. Please let me know if you don't find the time to provide a patch for the suggested fix. Created attachment 206802 [details]
proposed patch
I had something along the lines of the attached patch in mind. But I will first have to run it through the tests.
On another note: I have to say that I don't quite understand the point of #isExistingOrRenamedResourceURI(URI). It checks in the ResourceSet's #getURIResourceMap(). But is that any different from #hasDescription(URI) which calls resourceSet.getResource(uri, false)?
(In reply to comment #5) > Created attachment 206802 [details] > proposed patch Thanks your very much! The API tooling does not complain about the changed super type? > On another note: I have to say that I don't quite understand the point of > #isExistingOrRenamedResourceURI(URI). It checks in the ResourceSet's > #getURIResourceMap(). But is that any different from #hasDescription(URI) which > calls resourceSet.getResource(uri, false)? It's based on the assumption that a renamed resource / changed resource URI may be contained in the live resource set (e.g. renamed Xtend class). In that case, the original resource description from the index may not leak through this view. IIRC, the URI map is not cleared automatically by the resource / resource set if a resource is renamed thus the map can be used as a poor mans solution. It was implemented quite in a rush before the 2.1 release so I'm afraid it lacks some serious testing. > (In reply to comment #5) > Thanks your very much! The API tooling does not complain about the changed > super type? The API tooling does complain. But it shouldn't be a problem to maintain the super type. It'll just be a bit confusing :-) I suppose the alternative is to document it as a compatibility problem. Do you think anyone may depend on that? > > On another note: I have to say that I don't quite understand the point of > > #isExistingOrRenamedResourceURI(URI). It checks in the ResourceSet's > > #getURIResourceMap(). But is that any different from #hasDescription(URI) which > > calls resourceSet.getResource(uri, false)? > > It's based on the assumption that a renamed resource / changed resource URI may > be contained in the live resource set (e.g. renamed Xtend class). In that case, > the original resource description from the index may not leak through this > view. IIRC, the URI map is not cleared automatically by the resource / resource > set if a resource is renamed thus the map can be used as a poor mans solution. > It was implemented quite in a rush before the 2.1 release so I'm afraid it > lacks some serious testing. OK. Sounds sneaky. But if it works, it works :-) Created attachment 206805 [details]
updated patch
Updated patch which maintains the same supertype to not break the 2.1 API.
(In reply to comment #8) > Created attachment 206805 [details] > updated patch > > Updated patch which maintains the same supertype to not break the 2.1 API. Patch looks good at a first glance. Even though the super type may be irritating, I'd rather leave it as is and document it. Opinions? (In reply to comment #9) > Even though the super type may be irritating, I'd rather leave it as is and > document it. Opinions? Makes sense. (In reply to comment #10) > (In reply to comment #9) > > Even though the super type may be irritating, I'd rather leave it as is and > > document it. Opinions? > > Makes sense. Knut, do you have an idea when you'll find the time to add the clarifying comment and push the changes? (In reply to comment #11) > do you have an idea when you'll find the time to add the clarifying comment and > push the changes? I can do that today. I thought you were maybe waiting on feedback from others as well. Fix pushed to master: http://git.eclipse.org/c/tmf/org.eclipse.xtext.git/commit/?id=4bc19c8b181d540ec2de4fb17126f9eb5808dfe9 Closing all bugs that were set to RESOLVED before Neon.0 Closing all bugs that were set to RESOLVED before Neon.0 |