during the work on Connect for Life OpenMRS distribution we encountered some performance issues with OMRS HTML Forms, and we’ve found some improvements in the core OpenMRS code which I would like to share with you, get some feedback and maybe provide a contribution to the OpenMRS code base.
First some background information.
We’ve needed to create a form which would allow clinician to input information about up to 10 programs. Each program consists of a name (free text obs), reason (3 options obs), start and end dates (date widget obs) and up to 10 interruptions. Each interruption consists of reason (free text obs), start and end date times (date widget obs).
The OMRS HTML Forms obs can’t be added client-side, so we’ve used reapet-tag to create the form with 340 obs tags and then some JavaScript code to hide/show/style the page.
Generally, it would work. On the developer’s machine it would load in ~3 seconds, but once we deployed it to the AWS with webapp server and database on separate machines, the time sky-rocked to around 30-40 seconds for loading.
We’ve managed to cut the time to around 6 seconds with following changes/hacks:
Each obs tag in HTML form is handled on its own, that means for each of these 340 tags ConceptService#getConcept() is called, for each call an authorization check is performed.
Profiling the application, we’ve noticed that this check takes considerable part of the time. As a quick fix, we’ve added another service to get concept, which doesn’t make authorization check. The service will also cache the result - although caching might not be the most useful in this case (we’ve used that OMRS build-in caching).
Each obs tag with a date widget, will read two global properties - how many years to allow to choose from in date input and what is a date format. That two global properties are read from DB for every obs on the HTML form. Again, profiling the application, it showed up as having considerable impact. We’ve added that OMRS Caching annotations to the AdministrationService#getGlobalProperty(String).
The 1st hack seems to be sketchy, but the second change doesn’t look terrible.
Also, I’ve been wondering why such thing as Global Properties, which seems to be something which is going to be called a lot, is not loaded into memory in its entirety.
@pwargulak - thanks for the great post - all great ideas! I’d love to see improvements in this direction. When I read this, my first thought was - I thought these improvements were already made - but that isn’t entirely true. Let me share a few thoughts based on what I’ve seen and done:
This is a really good point, and we could certainly improve the caching of Concepts as you have done. I’d be more than happy to have improvements in HFE or in core that are introduced in this area. I had thought we already did this in the past, but what I found is that we already did some work around this for concepts that are referenced by mapping. Now, in my view all concepts should always be referenced by mapping, which is a better external reference than uuid, and certainly should be used over primary key id which is not portable across databases. You can see in the ObsSubmissionElement that all concept lookups are done by calling HtmlformentryUtil.getConcept(String lookup), and this will attempt to lookup by Mapping, which hits the HtmlFormEntryService#getConceptByMapping method, which leverages a cache that means that a given concept is only ever retrieved from the DB once.
If you use concept mappings on your form, do you notice improved peformance?
I totally agree! Actually, I added a feature to core around a year ago that aims to accomplish this. See ConfigUtil in core. This isn’t the easiest thing to use in HFE just because HFE supports earlier versions of core than where this was introduced, but we could easily add a class just like this to HFE to use (which would be phased out when the time comes when the minimum supported core version in HFE increases sufficiently).
Feel free to share your htmlforms so we can see how they are written - there may be other tricks we can help with.
@mseaton The ConfigUtil from openmrs-core looks promising, we’ll think about upgrading the core.
As for HtmlFormEntryServiceImpl#getConceptByMapping, I’m not sure if this is going to help, it seems to be caching the ‘ID to Mapping’, but then still makes a call to ConcetService#getConcept(Integer) - this was slow for us.
@pwargulak you are right that there are a lot of improvements that we could make here. Anything we can do that improves the performance of loading forms would be a welcome enhancement. That could be done either by improvements to core or to the htmlformentry module directly (or both). Do you have code that you have written that adds the appropriate caching and can be issued in a PR that we can review and incorporate?
From your comments above, it sounds like at least the following improvements would help considerably:
Caching results of privilege checks (eg. on the current thread or the current FormEntryContext/Session or within the UserContext itself).
Caching results of the HtmlformentryUtil.getConcept(String lookup) method, so any given lookup string is only ever fetched from the DB once per form load (either per request or potentially server-wide, though this requires handling cache invalidation if concepts are changed).
Potentially enhancing either AdministrationService to pre-load all global properties into memory, and update these by implementing GlobalPropertyListener when changed, or doing this in ConfigUtil.
These all seem like generally useful features in HFE and OpenMRS generally. I’m sure we could expand on this list to improve things in a range of areas. Should we start by creating some tickets and implementing each of these and you can use your use cases to provide some real-world validation as to how they do or don’t improve performance?