Add optional argument for making clone of predefined resources #1500
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
The FHIRDefinitions class provides a public method to return all predefined resources. Hypothetically, the caller of this method could alter the predefined resources, which is why it could be useful to return a clone of these resources. However, in all places this method is called by SUSHI, the resources are used for a search, not for any modifications. By not cloning the resources for the search, significant time savings are possible. Just in case someone else is using SUSHI as a dependency and calling this method and expecting to receive a clone, the default behavior is to return a clone. When searching for slices, the slice of an element is always a sibling of that element. Therefore, it is preferable to search the smaller list containing only the element's parent's children, when possible. This takes advantage of the underlying ElementDefinition tree. Because the root element's parent is undefined, use the full element list when working with the root element. This shouldn't be possible in most cases, as revealed by the test coverage, but the implementation is defensive just in case something unusual has happened to the root element.
jafeltra
approved these changes
Aug 16, 2024
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 PR applies to task CIMPL-1250.
The FHIRDefinitions class provides a public method to return all predefined resources. Hypothetically, the caller of this method could alter the predefined resources, which is why it could be useful to return a clone of these resources. However, in all places this method is called by SUSHI, the resources are used for a search, not for any modifications. By not cloning the resources for the search, significant time savings are possible. Just in case someone else is using SUSHI as a dependency and calling this method and expecting to receive a clone, the default behavior is to return a clone.
When searching for slices, the slice of an element is always a sibling of that element. Therefore, it is preferable to search the smaller list containing only the element's parent's children, when possible. This takes advantage of the underlying ElementDefinition tree. Because the root element's parent is undefined, use the full element list when working with the root element. This shouldn't be possible in most cases, as revealed by the test coverage, but the implementation is defensive just in case something unusual has happened to the root element.
A regression test of public FSH projects updated within the last year indicated no changes. The initial test run indicated changes in two repos, but both of those repos produced identical results in a second run without any code changes. The changes to
allPredefinedResourcesare most visible in projects with many StructureDefinitions. The changes toElementDefinitionare most visible in projects with many Instance definitions. The impact of theElementDefinitionchanges is relatively small, but theallPredefinedResourceschanges are significant for some FSH projects.The regression test results summary is attached as html. Three projects showed increased run times, but all of those projects are relatively small, with the longest run time being 61 seconds. Some larger projects showed meaningful decreases in run time. As a few examples:
While these are just a few examples I've picked out, and not every project has changes that are so dramatic, I think the overall effect is meaningfully positive without adding an odious amount of complexity to the implementation.
Regression summary HTML