Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JsonResourceImpl.getEObjectByID should not delegate to EMF when an id is not found #41

Closed
pcdavid opened this issue Jun 10, 2024 · 1 comment · Fixed by #42
Closed
Assignees

Comments

@pcdavid
Copy link
Member

pcdavid commented Jun 10, 2024

When JsonResourceImpl.getEObjectByID(String id) is called with an id which is not present in the resource and not found in the local idToEObjectMap, it delegates to the default implementation from EMF: return super.getEObjectByID(id).

However because we use our own idToEObjectMap instead of EMF's intrinsicIDToEObjectMap, that map (from EMF's ResourceImpl) is null, which makes EMF fallback to a very costly for (TreeIterator<EObject> i = getAllProperContents(getContents()); i.hasNext(); ) {...}.

This scenario is actually very common. For example if a project contains 25 documents, looking for an EObject in the last one (in ResourceSet.getResources() order) will try and fail, slowly, on the first 24 documents before finding the right one.

In the context of Sirius Web, DefaultObjectSearchService.getObject(IEditingContext, String) can be called a lot, with EObject ids (which will be found, even if in the "last" resource looked into), but also with representation ids of editing context ids, in which cases we will look into all EMF resources's contents for nothing before searching for representations/editing contexts.

@pcdavid
Copy link
Member Author

pcdavid commented Jun 10, 2024

I think we can assume that when JsonResourceImpl.useID is set, the idToEObjectMap is correctly filled with all the resource elements' ids (otherwise that's a bug) and if the requested id is not found there we can return null immediatly.

@pcdavid pcdavid self-assigned this Jun 10, 2024
pcdavid added a commit that referenced this issue Jun 10, 2024
EMF's default implementation is slow, and we can trust our own local
idToEObjectMap.

Bug: #41
Signed-off-by: Pierre-Charles David <[email protected]>
@pcdavid pcdavid linked a pull request Jun 10, 2024 that will close this issue
sbegaudeau pushed a commit that referenced this issue Jun 16, 2024
EMF's default implementation is slow, and we can trust our own local
idToEObjectMap.

Bug: #41
Signed-off-by: Pierre-Charles David <[email protected]>
pcdavid added a commit that referenced this issue Jun 26, 2024
The previous commit (1f44029) completely disabled EMF's default
getEObjectByID in favor of using our own JSONResourceImpl-specific id
map. However there are cases where we still want to lookup elements by
their intrinsic (modeled) id instead of the IDManager-provided one,
without incurring the cost of an eAllContents() on failure.

This change allows for this to work, as long as the JSONResourceImpl
has been setup with a non-null intrinsicIDToEObjectMap. For example
right after the resource creation:

  ((ResourceImpl) resource).setIntrinsicIDToEObjectMap(new HashMap<>());

Once this is done, any new object added to the resource will get its
intrinsic id cached in this map (see ResourceImpl.attachedHelper())
and JsonResourceImpl.getEObjectById will find it there (if present)
while still bypassing the whole eAllContents() search in
super.getEObjectByID(id).

Bug: #41
Signed-off-by: Pierre-Charles David <[email protected]>
pcdavid added a commit that referenced this issue Jun 26, 2024
The previous commit (1f44029) completely disabled EMF's default
getEObjectByID in favor of using our own JSONResourceImpl-specific id
map. However there are cases where we still want to lookup elements by
their intrinsic (modeled) id instead of the IDManager-provided one,
without incurring the cost of an eAllContents() on failure.

This change allows for this to work, as long as the JSONResourceImpl
has been setup with a non-null intrinsicIDToEObjectMap. For example
right after the resource creation:

  ((ResourceImpl) resource).setIntrinsicIDToEObjectMap(new HashMap<>());

Once this is done, any new object added to the resource will get its
intrinsic id cached in this map (see ResourceImpl.attachedHelper())
and JsonResourceImpl.getEObjectById will find it there (if present)
while still bypassing the whole eAllContents() search in
super.getEObjectByID(id).

Bug: #41
Signed-off-by: Pierre-Charles David <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant