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

Add memoization to toFHIRObject #44

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mrnosal
Copy link

@mrnosal mrnosal commented Aug 8, 2024

Add simple memoization function based on Map() to improve performance of toFHIRObject when executing complex CQL on large FHIR resource bundles.

const memoize = (fn) => {
let cache = new Map();
return (...args) => {
let cacheKey = JSON.stringify([...args]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This key can likely get quite large since it's stringifying the passed in data, typeSpecifier, modelInfo, and suffix.

In particular, I thought modelInfo was going to be a big problem because the modelInfo is quite large with several duplicative maps in it -- but running it through the debugger, I see that JSON.stringify converts all those huge Maps into empty objects ({ }), which turns out to be helpful for us. Still, modelInfo should be static and identifiable by its url and version -- so we could update the key to only use that instead of the whole modelInfo. Of course then this is no longer a fully generic memoization function, as it would need to be type-aware.

I'm not sure how to get around the problem when data is huge though (for example, if this were being called on a FHIR Bundle with hundreds of embedded resources). In theory, if the data is a resource, then the resourceType/id combination would usually be unique, but there could be extreme edge cases where maybe that's not the case.

Might it make sense to hash the big string and use the hash as the key? I don't remember how expensive a hash operation is though, so I'm not sure how to compare efficiency of having to store very large string keys in the map vs having to hash those strings on every call. I bet @birick1 knows.

@mrnosal mrnosal closed this Aug 15, 2024
@mrnosal mrnosal reopened this Aug 15, 2024
@cmoesel
Copy link
Member

cmoesel commented Oct 21, 2024

@mrnosal - what is the status on this PR? Are you waiting on me? Or am I waiting on you? Or are we both waiting on a discussion?

@mrnosal
Copy link
Author

mrnosal commented Oct 21, 2024

@cmoesel We should discuss. Stringify is native, and hashing is slower. Trade-offs between overhead for small bundles vs. savings for large bundles, but do want to move performance forward.

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.

2 participants