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

Unexpected performance hit when switching from EmfModel to InMemoryEmfModel #45

Open
agarciadom opened this issue Jul 5, 2023 · 1 comment
Labels
bug Something isn't working

Comments

@agarciadom
Copy link
Contributor

agarciadom commented Jul 5, 2023

Working on the TTC KMEHR to FHIR case today, I noticed that the benchmark driver in its reference solution, transforms a File into a Resource, rather than a File to a File. This is done so the "Run" phase of the measurements does not include the time used in saving the model.

To make results comparable, I decided to make the same change, and have the ETL transformation go from an EmfModel to an InMemoryEmfModel. When I did that, however, I noticed it significantly slowed down. VisualVM points to the maintenance of the allContents cache:

image

This wasn't an issue with EmfModel. It turns out that at some point, I added some code to register CachedContentsAdapters automatically in the initialisation of InMemoryEmfModel. I wonder why I didn't check whether caching was enabled or not at that time - I can't remember at the moment.

Later on, Sina changed the code to just use setCachingEnabled(true), which performs the same work but is also consistent with the cached flag. This is after a commit where he fixed EmfModel::setCachingEnabled to add/remove the CachedContentsAdapter itself (as it should have).

Looking at this again, I wonder if we should drop this altogether from InMemoryEmfModel, and just let users decide if they want to turn on caching or not by themselves:

  // Since 1.6, having CachedContentsAdapter implies cached=true, otherwise it's inconsistent.
  setCachingEnabled(true);
@agarciadom agarciadom added the bug Something isn't working label Jul 5, 2023
@agarciadom agarciadom added this to the 2.5.0 milestone Jul 5, 2023
@arcanefoam
Copy link
Contributor

I think the user should always decide. If not, users will see/perceive the performance/memory hit and be confused if they are not selecting the cached option.

@agarciadom agarciadom removed this from the 2.5.0 milestone Nov 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants