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

Performance update for 1.0 #590

Merged
merged 4 commits into from
Oct 21, 2024
Merged

Conversation

DamienMosier
Copy link
Contributor

@DamienMosier DamienMosier commented Oct 8, 2024

Current 1.0

ADD-E: Duration: 3h 32m 35s
CRE: Duration: 2h 46m 1s
PND-E: Duration: 10h 48m 39s
SPD: Duration: 3h 10m 21s

Time to run 80 HEDIS measures with multiple configurations using current develop 1.0 branch
image

With these updates, same build pipeline

ADD-E: Duration: 38m 0s
CRE: Duration: 57m 4s
PND-E: Duration: 53m 39s
SPD: Duration: 1h 36m 22s

image

@ewoutkramer
Copy link
Member

@baseTwo - is this something we need in your equivalent 2.0 port as well?

@baseTwo
Copy link
Collaborator

baseTwo commented Oct 17, 2024

@baseTwo - is this something we need in your equivalent 2.0 port as well?

We have an issue to add memoization as part of the context #600, however, it looks like 1.0 changed back to the old way where most libraries contain state for the context and the Lazy<>'s.

@DamienMosier , which measures do not have context defined, I'd like to compare the two types, but GitHub collapses too many files for me to go through each one until I find it.

@DamienMosier
Copy link
Contributor Author

@ewoutkramer this is something you would definitely want in the 2.0 branch for the performance gains.

@baseTwo The way the logic work is if the file or one of the includes has a context then it creates it the old way with a constructor and utilizes Lazy. If the file does not, aka a library, then it's a singleton.

For our use case only our actual measures have context Patient and all of our libraries are either functions or fluent functions with the resources passed in. In your Demo project, context is added to almost all files which is why it looks like the old way.

The main performance issue with the original 1.0 branch was with the constructors. If you have LibraryA that has 5 includes and each of those subsequent libraries each have their own set of includes. You could end up instantiating the same library multiple times. Having it scoped allowed us to use Lazy so we gained the benefit of some caching but at the expense of calling the constructors a lot. Switching to singleton removed that constructor lift but removed the ability to use Lazy. I thought about it and realized we only need Lazy where there is context and all subsequent libraries did not need it and we could have the best of both worlds.

Here are some partial examples of measure AAB from DQIC if you want to see the CQL.

[System.CodeDom.Compiler.GeneratedCode(".NET Code Generation", "1.0.0.0")]
[CqlLibrary("AAB_Concepts", "2024.1.0")]
public class AAB_Concepts_2024_1_0
{

    public static AAB_Concepts_2024_1_0 Instance { get; }  = new();

    #region Dependencies
    #endregion

    [CqlDeclaration("Acute Bronchitis")]
    [CqlValueSet("https://www.ncqa.org/fhir/valueset/2.16.840.1.113883.3.464.1004.1016")]
	public CqlValueSet Acute_Bronchitis(CqlContext context) => 
		new CqlValueSet("https://www.ncqa.org/fhir/valueset/2.16.840.1.113883.3.464.1004.1016", null);

    [CqlDeclaration("Competing Diagnosis")]
    [CqlValueSet("https://www.ncqa.org/fhir/valueset/2.16.840.1.113883.3.464.1004.1067")]
	public CqlValueSet Competing_Diagnosis(CqlContext context) => 
		new CqlValueSet("https://www.ncqa.org/fhir/valueset/2.16.840.1.113883.3.464.1004.1067", null);

and the actual file with context is reverted to the old way, the same way you see files in the DQIC repository.

[System.CodeDom.Compiler.GeneratedCode(".NET Code Generation", "1.0.0.0")]
[CqlLibrary("AAB_Reporting", "2024.1.0")]
public class AAB_Reporting_2024_1_0
{
    internal CqlContext context;

    #region Cached values

    internal Lazy<Tuples.Tuple_BOFQdLfCiMegGCeQZaSIJRaC> __Measure;
    internal Lazy<Patient> __Patient;
    internal Lazy<IEnumerable<ExplanationOfBenefit>> __Eobs;
    internal Lazy<IEnumerable<ExplanationOfBenefit>> __Admin_Eobs;
    
    #endregion
    public AAB_Reporting_2024_1_0(CqlContext context)
    {
        this.context = context ?? throw new ArgumentNullException("context");
        
        __Measure = new Lazy<Tuples.Tuple_BOFQdLfCiMegGCeQZaSIJRaC>(this.Measure_Value(context));
        __Patient = new Lazy<Patient>(this.Patient_Value(context));
        __Eobs = new Lazy<IEnumerable<ExplanationOfBenefit>>(this.Eobs_Value(context));
        __Admin_Eobs = new Lazy<IEnumerable<ExplanationOfBenefit>>(this.Admin_Eobs_Value(context));

@DamienMosier
Copy link
Contributor Author

DamienMosier commented Oct 17, 2024

For reference, I generated this for Evan after doing testing. The number are based off our build machine times. The "1.0 DCS" is the original 1.0 prior to singleton. The "1.0 Develop" is current develop with only singleton's and the final are these changes that are pending.

image

@ewoutkramer ewoutkramer merged commit fa542d6 into FirelyTeam:develop Oct 21, 2024
2 of 4 checks passed
@DamienMosier DamienMosier deleted the performance branch October 21, 2024 11:08
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 this pull request may close these issues.

4 participants