Community
Participate
Working Groups
The default strategy to check, whether all links are resolvable is to rely on EcoreUtil.resolveAll. This enforces all proxies to be resolved. However when working with the index we often know about the existence of an EObject without having to load it into memory. We should come up with an explicit linking validation, which is based on ILinkingService but takes any returned EObjects or Proxies as valid cross references without trying to resolve the returned proxies.
Added a method to LazyLinkingResource, which is capable of resolving lazy links only and not transitively. It optionally also replaces lazy linking proxies with the resolved elements, but as it does it from outside the notification fired is a Notficiation.SET and not a Notification.RESOLVE (which would be wrong anyway, since the new element might be a proxy as well). I tried to make that explicit through JavaDoc and the additional parameter. The only client for this code is ResourceValidatorImpl, which uses this approach and replaces the resolved elements. I suspect that most clients don't rely on the notifications and if they rely on it, they need to change the ResourceValidatorImpl to fall back to the old more expensive EcoreUtil.resolveAll. I've disabled the cache eviction for the resolution of lazy links. Pushed to master.
Any words about the impact on the performance of the linking validation phase?
Created attachment 183619 [details] patch for many references It doesn't look like the new parameter replaceResolvedElements is being used. I also found a problem with how the code deals with many references. Whether eGet() is called with "resolve" set to true or false doesn't make a difference. Important is how the list elements are subsequently accessed. See attached patch and test case for details. Further I don't think that a RESOLVE event would be wrong even if the returned object is still a proxy. That's after all also what EMF normally would do if it could resolve a proxy to another proxy only. So another solution would be to during validation resolve the lazy link proxies only and then block EMF's recursive proxy resolution from resolving any further. This behavior would of course be specific to IResourceValidator#resolveProxies() only (or any other clients specifically requesting that behavior).
Thanks, I've applied the patch. I also deactivated notification. Regarding performance: It is as fast as the old approach in most scenarios. Only if loading a referenced resource would be time consuming, because it is super large or not locally available it would make a noticeable difference I guess.
What about the EcoreUtil.resolveAll() call in DefaultResourceDescription#getImportedNames()? I suppose that could also be changed to use LazyLinkingResource#resolveLazyCrossReferences(), or did I miss something?
Yes, of course. Could you take care of it?
OK. I will move the logic of ResourceValidatorImpl#resolveProxies() into EcoreUtil2#resolveAll() unless you can see a problem with that approach. The only other client of EcoreUtil2#resolveAll() is ReconcilingUnitOfWork#exec(), but I can't see how that would hurt there.
(In reply to comment #7) > OK. I will move the logic of ResourceValidatorImpl#resolveProxies() into > EcoreUtil2#resolveAll() unless you can see a problem with that approach. The > only other client of EcoreUtil2#resolveAll() is ReconcilingUnitOfWork#exec(), > but I can't see how that would hurt there. The logic doesn't "resolve all", hence it shouldn't be part of a method which is named that way. I'ld prefer adding a method 'resolveLazyReferences' to EcoreUtil2.
Hi Knut, the ReconcilingUnitOfWork should resolve the proxies instead of ensuring that they exist. I'd assume that some fragile or offset / list index sensitive fragment provider implementations may not be able to resolve platform:/resource/... uris after something has been modified in the resource itself. Regards, Sebastian
Pushed fix as discussed to master.
Just as a note: This enhancement lowered our build times by about 10%.
I'm reopening this one for the following reasons: 1. The custom EObjectValidator added by the CompositeEValidator still causes all cross references to be resolved. That is because of the inherited check validate_EveryReferenceIsContained(). I suggest we override that one just like we do with validate_EveryProxyResolves(). 2. ClusteringBuilderState#doUpdate() still calls EcoreUtil2.resolveAll(). We should replace this with EcoreUtil2.resolveLazyCrossReferences(). Also note that DefaultResourceDescriptionStrategy.createReferenceDescriptions() (which is of course called during the build) will also cause all cross references to be (fully) resolved, as it calls EObject.eGet() for all cross references. This one is although a bit tougher to solve: We would have to adapt DefaultResourceDescriptionStrategy.isResolvedAndExternal() to check if the target object is an unresolved lazy linking proxy (e.g. using LazyLinkingResource.getEncoder().isCrossLinkFragment()), in which case no reference description should be created. Further it is of course possible that the linking service returns proxies which are intended to be further resolved (automatically by EcoreUtil.resolve()) and the reference description should then refer to the fully resolved target. But I think in that case the user would have override the default implementation to deal with that. I suppose it might make sense to add a method like LazyLinkingResource.isUnresolvedReference(EObject, EReference, EObject) (or simply LazyLinkingResource.isUnresolvedReference(URI)) for this purpose.
Created attachment 190955 [details] resolve references patch The attached patch solves the first two issues described in my last comment: It replaces the call to EcoreUtil2#resolveAll() in ClusteredBuilderState and it overrides the EObjectValidator#validate_EveryReferenceIsContained() method in CompositeEValidator. Any thoughts on what could / should be done with DefaultResourceDescriptionStrategy#isResolvedAndExternal() ?
(In reply to comment #13) > Any thoughts on what could / should be done with > DefaultResourceDescriptionStrategy#isResolvedAndExternal() ? We could try to resolve it with the same logic as in resolveLazyCrossReferences and check the resulting URI. If it is a platform:/ uri and points to another resource, it's safe to assume that it's resolved and external.
Knut, I don't like the idea to override #validate_EveryReferenceIsContained as it misses important problems. Do you mind to override this one for your specific project by means of #isUseEObjectValidator == false and providing your own EObject validator?
(In reply to comment #15) > Knut, I don't like the idea to override #validate_EveryReferenceIsContained as > it misses important problems. Do you mind to override this one for your > specific project by means of #isUseEObjectValidator == false and providing your > own EObject validator? That sounds reasonable. Although it sounds like a quite esoteric use case for a reference in an Xtext resource to reference a non-contained object.
(In reply to comment #16) > That sounds reasonable. Although it sounds like a quite esoteric use case for a > reference in an Xtext resource to reference a non-contained object. Exactly. That's why I'd like to have the appropriate validation rule available by default. Power users are free to override it and others won't be harmed when referenced resources are loaded in the validation stage. Knut, could you please update the suggested patches and apply them?
Created attachment 193220 [details] lazy resolution of references in DefaultResourceDescriptionStrategy (In reply to comment #14) > (In reply to comment #13) > > Any thoughts on what could / should be done with > > DefaultResourceDescriptionStrategy#isResolvedAndExternal() ? > > We could try to resolve it with the same logic as in resolveLazyCrossReferences > and check the resulting URI. If it is a platform:/ uri and points to another > resource, it's safe to assume that it's resolved and external. IMO the URI check should by default instead check if the URI is not a lazy proxy URI. See attached patch for details. Further I think it would be cleaner to add something like a XtextResource#isResolved() method to encapsulate this bit.
Hi Knut, the patch looks good to me. Please apply. I wouldn't introduce a dependency to the XtextResource to encapsulate this information. I like the fact that the default resource description strategy works nicely with all kinds of emf resources.
Would it be possible to add a test case that documents the lazy cross reference resolution in the default resource description strategy, e.g. one that shows that a referenced resource will not be loaded into the resource set for the sake of creating reference descriptions?
(In reply to comment #20) > Would it be possible to add a test case that documents the lazy cross reference > resolution in the default resource description strategy, e.g. one that shows > that a referenced resource will not be loaded into the resource set for the > sake of creating reference descriptions? Yes, I will add a test case for that. That will be useful.
pushed to master.
Closing all bugs that were set to RESOLVED before Neon.0
