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.

Sign up for GitHub

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

Jump to bottom

Add memoization to toFHIRObject #44

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

Add memoization to toFHIRObject #44

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion src/fhir.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,20 @@ const FHIRv300XML = require('./modelInfos/fhir-modelinfo-3.0.0.xml.js');
const FHIRv400XML = require('./modelInfos/fhir-modelinfo-4.0.0.xml.js');
const FHIRv401XML = require('./modelInfos/fhir-modelinfo-4.0.1.xml.js');

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.

if (cache.has(cacheKey)) {
return cache.get(cacheKey);
} else {
let result = fn(...args);
cache.set(cacheKey, result);
return result;
}
}
}

class FHIRWrapper {
constructor(filePathOrXML) {
this._modelInfo = load(filePathOrXML);
Expand Down Expand Up @@ -496,7 +510,9 @@ function toSystemObject(data, name) {
* @param {string} suffix - the trailing part of the path to get (e.g., x.y.z)
* @returns {FHIRObject}
*/
function toFHIRObject(data, typeSpecifier, modelInfo, suffix) {
const toFHIRObject = memoize(_toFHIRObject);

function _toFHIRObject(data, typeSpecifier, modelInfo, suffix) {
if (data == null) {
// preserve distinction between null or undefined
return data;
Expand Down
Loading