FirelyTeam / firely-cql-sdk

BSD 3-Clause "New" or "Revised" License
31 stars 17 forks source link

Context TypeConverter fix #512

Closed DamienMosier closed 2 months ago

DamienMosier commented 2 months ago

Fix for https://github.com/FirelyTeam/firely-cql-sdk/issues/511

Pass in TypeConverter when creating a context to avoid the recreation for new contexts with new datasources

DamienM419 commented 2 months ago

after the fix the total execution time dropped to 1.73% vs 27.9%

image

richfirely commented 2 months ago

You know its funny this just came about, because I was just looking at the performance of the DQIC test decks and thought to look into this someday. I tested this PR change in 2.0 and I see the large performance benefit. Test time actually decreased by 35% for me.

I am curious, since the code doesn't change the value of the TypeConverter before this PR, can we instead create the static field inside FhirModelBindingSetup to avoid having to pass it in all the time?

I am just trying to think about how it becomes obvious that you will have a performance issue if you don't pass your own instance. This is more educational for me than questioning anything in this PR. I look forward to the improvement.

DamienM419 commented 2 months ago

I think the problem with that would be the way the context is setup using ForBundle in a loop so each patient has it's own context. Once you dig 2 more layers deep from ForBundle you get to the creation of the CqlContext which gets a new context and we call new on the FhirModelBindingSetup. I don't think using static here would do anything because we are creating a new instance of the class every time. To minimize the changes to the 1.0 branch I chose to just add it as a parameter.

internal static CqlContext createContext(IDataSource? dataSource = null, IDictionary<string, object>? parameters = null, IValueSetDictionary? valueSets = null, DateTimeOffset? now = null, DefinitionDictionary? delegates = null, FhirModelBindingOptions? options = null, TypeConverter? typeConverter = null) => new CqlContext( new FhirModelBindingSetup(dataSource, valueSets, now, options, typeConverter).Operators, parameters, delegates);

ewoutkramer commented 2 months ago

I can see how this fixes the performance problem, but it means we're putting the responsibility of managing the lifetime of the TypeConverter in the user's hands. Just taking a step back, does it make sense the user needs to deal with that? The typeconverter does feel like a singleton to me, so what is it in the typeconverter that we now need to manage its lifetime as an end user?

DamienM419 commented 2 months ago

There is nothing in the typeconverter that the end user needs to manage. This fix was only implemented to reduce code churn on a locked 1.0 branch while improving performance, hence why it's an optional parameter. Redesigning the architecture into a singleton and how the context was being created seemed more like a 2.0 branch update.

DamienM419 commented 2 months ago

@ewoutkramer I revisited the changes and saw that the only reason Create was being called was to set the LRU Cache so I created an initializer for that and returned to using the singleton "Default". The user will no longer have to manage the TypeConverter and this should be a cleaner updated.