[43] Only call getPackageForURI on URI-like paths #45
+14
−14
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This avoids creating thousands of PackageNotFoundError instances when resolving "normal" inter-document references which do not refer to EPackages via nsURIs. PackageNotFoundError are much cheaper to create than the previously used PackageNotFoundException, but that's still a lot of garbage instances to create, add into the resource.getErrors() list and never use (or even clear). In addition, in the context of Sirius Web, every instance will have its own duplicate string representation of the failed "package" URI because we get it from EObjectIDManager.getOrCreateId(EObject) which invokes UUID.toString() and allocates a new (identical) String each time. Bug: #43 Signed-off-by: Pierre-Charles David <[email protected]>
pcdavid
commented
Jun 26, 2024
Comment on lines
+547
to
554
| if (resourceURIPath.indexOf(SCHEME_SEPARATOR) != -1) { | ||
| EPackage ePackage = this.getPackageForURI(resourceURIPath); | ||
| if (ePackage != null) { | ||
| object = ePackage.eResource().getEObject(fragmentEMF); | ||
| } | ||
| } else { | ||
| object = this.createProxyEObject(id, qualifiedType, eReference); | ||
| } |
There was a problem hiding this comment.
I think the new logic is wrong. It assumes only resources from the package registry have full URI paths (with scheme, separator, etc.). This will not be the case in e.g. SysON where standard libraries, which are plain models, have URIs of the form kermllibrary://<uuid>. With the new code, such a URI would be lookup up in the package registry (because is has a :), which will fail, and we'll stop there. We should fall back to the this.createProxyEObject() in this case as in a non-URI-like path.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This avoids creating thousands of PackageNotFoundError instances when
resolving "normal" inter-document references which do not refer to
EPackages via nsURIs.
PackageNotFoundError are much cheaper to create than the previously
used PackageNotFoundException, but that's still a lot of garbage
instances to create, add into the resource.getErrors() list and never
use (or even clear).
In addition, in the context of Sirius Web, every instance will have
its own duplicate string representation of the failed "package" URI
because we get it from EObjectIDManager.getOrCreateId(EObject) which
invokes UUID.toString() and allocates a new (identical) String each
time.
Bug: #43
Signed-off-by: Pierre-Charles David [email protected]