Skip to content

FLAG-69: Improve the performance of patient flag evaluation #59

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

Open
ManojLL wants to merge 3 commits into
base: master
Choose a base branch
from
Open

FLAG-69: Improve the performance of patient flag evaluation #59

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b9a9d6d
FLAG 69 : resolve conflict
ManojLL Jun 24, 2024
a1e4aa6
FLAG 69 : refactor generate patient flag on parallel
ManojLL Jun 24, 2024
8d3a634
FLAG 69: refactor the code using forEach
ManojLL Jun 26, 2024
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
47 changes: 32 additions & 15 deletions api/src/main/java/org/openmrs/module/patientflags/api/impl/FlagServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.Future;
import java.util.concurrent.TimeUnit;
import java.util.stream.Collectors;

/**
Expand Down Expand Up @@ -95,23 +96,41 @@ public List<Flag> generateFlagsForPatient(Patient patient, Map<Object, Object> c
/**
* @see org.openmrs.module.patientflags.api.FlagService#generateFlagsForPatient(Patient, Filter, Map<Object, Object>)
*/
public List<Flag> generateFlagsForPatient(Patient patient, Filter filter, Map<Object, Object> context) {
List<Flag> results = new ArrayList<Flag>();
public List<Flag> generateFlagsForPatient(final Patient patient, Filter filter, final Map<Object, Object> context) {
final List<Flag> results = new ArrayList<Flag>();

// we can get rid of this once onStartup is implemented
if (!isInitialized)
refreshCache();


executor = Executors.newFixedThreadPool(Runtime.getRuntime().availableProcessors());
// test each Flag in the cache against the specific Patient
for (Flag flag : filter.filter(flagCache)) {
for (final Flag flag : filter.filter(flagCache)) {
Copy link
Member

@wikumChamith wikumChamith Jun 26, 2024

Choose a reason for hiding this comment

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

Can we switch to streams here?

Copy link
Contributor Author

@ManojLL ManojLL Jun 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

updated the code with forEach

// trap bad flags so that they don't hang the system
try {
if (flag.eval(patient, context))
results.add(flag);
}
catch (Exception e) {
log.error("Unable to test flag " + flag.getName() + " on patient #" + patient.getId(), e);
executor.submit(() -> {
try {
Context.openSession();
if (flag.eval(patient, context)) {
synchronized (results) {
results.add(flag);
}
}
} catch (Exception e) {
log.error("Unable to test flag " + flag.getName() + " on patient #" + patient.getId(), e);
} finally {
Context.closeSession();
}
});
}

executor.shutdown();
try {
if (!executor.awaitTermination(Integer.MAX_VALUE, TimeUnit.NANOSECONDS)) {
log.error("Executor service did not terminate");
}
} catch (InterruptedException e) {
log.error("Thread pool was interrupted while waiting for flag evaluation tasks to complete", e);
Thread.currentThread().interrupt();
}
return results;
}
Expand Down Expand Up @@ -688,10 +707,8 @@ private List<Flag> getFlags(List<PatientFlag> patientFlags) {

@Override
public Future<?> evaluateAllFlags() {
if (executor == null) {
executor = Executors.newSingleThreadExecutor();
}

executor = Executors.newSingleThreadExecutor();
Copy link
Member

@wikumChamith wikumChamith Jun 26, 2024

Choose a reason for hiding this comment

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

why did you remove the null check here?

Copy link
Contributor Author

@ManojLL ManojLL Jun 26, 2024

Choose a reason for hiding this comment

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

in this line , I initialized the executor as a thread pool. If the generateFlagForPatient method is called first, the executor is already initialized as thread pool, so then evaluateAllFlags uses a thread pool instead of a single thread. after removing the null check when evaluateAllFLag method calls it initialize the executor as a single thread.


return executor.submit(PatientFlagTask.evaluateAllFlags());
}

Expand Down