From c5c3cc62fbe197a820f6c5ed889cb3b3d955da9f Mon Sep 17 00:00:00 2001 From: David Maplesden Date: Tue, 10 Sep 2019 11:11:45 +1200 Subject: [PATCH 1/7] Dramatically improve the performance of containResourcesForEncoding We do this by avoiding the double processing of contained resources when looking for resources to contain --- .../java/ca/uhn/fhir/parser/BaseParser.java | 83 ++++++++++++++++++- 1 file changed, 82 insertions(+), 1 deletion(-) diff --git a/hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/BaseParser.java b/hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/BaseParser.java index 9934bdade7e6..8226600c24a3 100644 --- a/hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/BaseParser.java +++ b/hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/BaseParser.java @@ -27,6 +27,7 @@ import ca.uhn.fhir.rest.api.Constants; import ca.uhn.fhir.rest.server.exceptions.InternalErrorException; import ca.uhn.fhir.util.UrlUtil; + import com.google.common.base.Charsets; import org.apache.commons.lang3.StringUtils; import org.apache.commons.lang3.Validate; @@ -235,7 +236,7 @@ private void containResourcesForEncoding(ContainedResources theContained, IBaseR } } - List allReferences = myContext.newTerser().getAllPopulatedChildElementsOfType(theResource, IBaseReference.class); + List allReferences = getAllBaseReferences(theResource); for (IBaseReference next : allReferences) { IBaseResource resource = next.getResource(); if (resource == null && next.getReferenceElement().isLocal()) { @@ -280,6 +281,86 @@ protected void containResourcesForEncoding(IBaseResource theResource) { } + protected List getAllBaseReferences(IBaseResource theResource){ + final ArrayList retVal = new ArrayList(); + findBaseReferences(retVal, theResource, myContext.getResourceDefinition(theResource)); + return retVal; + } + + /** + * A customised traversal of the tree to find the 'top level' base references. Nested references are found via the recursive traversal + * of contained resources. + */ + protected void findBaseReferences(List allElements, IBase theElement, BaseRuntimeElementDefinition theDefinition) { + if (theElement instanceof IBaseReference) { + allElements.add((IBaseReference) theElement); + } + + BaseRuntimeElementDefinition def = theDefinition; + if (def.getChildType() == ChildTypeEnum.CONTAINED_RESOURCE_LIST) { + def = myContext.getElementDefinition(theElement.getClass()); + } + + switch (def.getChildType()) { + case ID_DATATYPE: + case PRIMITIVE_XHTML_HL7ORG: + case PRIMITIVE_XHTML: + case PRIMITIVE_DATATYPE: + // These are primitive types + break; + case RESOURCE: + case RESOURCE_BLOCK: + case COMPOSITE_DATATYPE: { + BaseRuntimeElementCompositeDefinition childDef = (BaseRuntimeElementCompositeDefinition) def; + for (BaseRuntimeChildDefinition nextChild : childDef.getChildrenAndExtension()) { + + List values = nextChild.getAccessor().getValues(theElement); + if (values != null) { + for (Object nextValueObject : values) { + IBase nextValue; + try { + nextValue = (IBase) nextValueObject; + } catch (ClassCastException e) { + String s = "Found instance of " + nextValueObject.getClass() + " - Did you set a field value to the incorrect type? Expected " + IBase.class.getName(); + throw new ClassCastException(s); + } + if (nextValue == null) { + continue; + } + if (nextValue.isEmpty()) { + continue; + } + BaseRuntimeElementDefinition childElementDef; + childElementDef = nextChild.getChildElementDefinitionByDatatype(nextValue.getClass()); + + if (childElementDef == null) { + childElementDef = myContext.getElementDefinition(nextValue.getClass()); + } + + if (nextChild instanceof RuntimeChildDirectResource) { + // Don't descend into embedded resources + if (nextValue instanceof IBaseReference) { + allElements.add((IBaseReference) nextValue); + } + } else { + findBaseReferences(allElements, nextValue, childElementDef); + } + } + } + } + break; + } + case CONTAINED_RESOURCES: + // skip contained resources when looking for resources to contain + break; + case CONTAINED_RESOURCE_LIST: + case EXTENSION_DECLARED: + case UNDECL_EXT: { + throw new IllegalStateException("state should not happen: " + def.getChildType()); + } + } + } + private String determineReferenceText(IBaseReference theRef, CompositeChildElement theCompositeChildElement) { IIdType ref = theRef.getReferenceElement(); if (isBlank(ref.getIdPart())) { From b731f9c0638629047f5a76cd6ea6f04c4bbb7030 Mon Sep 17 00:00:00 2001 From: David Maplesden Date: Tue, 10 Sep 2019 12:41:16 +1200 Subject: [PATCH 2/7] Cache the composite children that need encoding to improve performance --- .../java/ca/uhn/fhir/parser/BaseParser.java | 170 ++++++++++-------- 1 file changed, 97 insertions(+), 73 deletions(-) diff --git a/hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/BaseParser.java b/hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/BaseParser.java index 8226600c24a3..c9d433a89b8a 100644 --- a/hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/BaseParser.java +++ b/hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/BaseParser.java @@ -68,6 +68,47 @@ public abstract class BaseParser implements IParser { private boolean mySuppressNarratives; private Set myDontStripVersionsFromReferencesAtPaths; + private Map> compositeChildrenCache = new HashMap<>(); + + private static class Key { + private final BaseRuntimeElementCompositeDefinition resDef; + private final boolean theContainedResource; + private final CompositeChildElement theParent; + private final EncodeContext theEncodeContext; + + public Key(BaseRuntimeElementCompositeDefinition resDef, final boolean theContainedResource, final CompositeChildElement theParent, EncodeContext theEncodeContext) { + this.resDef = resDef; + this.theContainedResource = theContainedResource; + this.theParent = theParent; + this.theEncodeContext = theEncodeContext; + } + @Override + public int hashCode() { + final int prime = 31; + int result = 1; + result = prime * result + ((resDef == null) ? 0 : resDef.hashCode()); + result = prime * result + (theContainedResource ? 1231 : 1237); + result = prime * result + ((theParent == null) ? 0 : theParent.hashCode()); + result = prime * result + ((theEncodeContext == null) ? 0 : theEncodeContext.hashCode()); + return result; + } + + @Override + public boolean equals(final Object obj) { + if (this == obj) { + return true; + } + if (obj instanceof Key) { + final Key that = (Key) obj; + return Objects.equals(this.resDef, that.resDef) && + this.theContainedResource == that.theContainedResource && + Objects.equals(this.theParent, that.theParent) && + Objects.equals(this.theEncodeContext, that.theEncodeContext); + } + return false; + } + } + /** * Constructor */ @@ -129,85 +170,37 @@ public IParser setEncodeElements(Set theEncodeElements) { } protected Iterable compositeChildIterator(IBase theCompositeElement, final boolean theContainedResource, final CompositeChildElement theParent, EncodeContext theEncodeContext) { - BaseRuntimeElementCompositeDefinition elementDef = (BaseRuntimeElementCompositeDefinition) myContext.getElementDefinition(theCompositeElement.getClass()); - final List children = elementDef.getChildrenAndExtension(); - - return new Iterable() { - - @Override - public Iterator iterator() { - - return new Iterator() { - private Iterator myChildrenIter; - private Boolean myHasNext = null; - private CompositeChildElement myNext; - - /** - * Constructor - */ { - myChildrenIter = children.iterator(); - } - - @Override - public boolean hasNext() { - if (myHasNext != null) { - return myHasNext; - } - - myNext = null; - do { - if (myChildrenIter.hasNext() == false) { - myHasNext = Boolean.FALSE; - return false; - } - - myNext = new CompositeChildElement(theParent, myChildrenIter.next(), theEncodeContext); - - /* - * There are lots of reasons we might skip encoding a particular child - */ - if (myNext.getDef().getElementName().equals("id")) { - myNext = null; - } else if (!myNext.shouldBeEncoded(theContainedResource)) { - myNext = null; - } else if (myNext.getDef() instanceof RuntimeChildNarrativeDefinition) { - if (isSuppressNarratives() || isSummaryMode()) { - myNext = null; - } else if (theContainedResource) { - myNext = null; - } - } else if (myNext.getDef() instanceof RuntimeChildContainedResources) { - if (theContainedResource) { - myNext = null; - } - } - } while (myNext == null); + return compositeChildrenCache.computeIfAbsent(new Key(elementDef, theContainedResource, theParent, theEncodeContext), (k) -> { + + final List children = elementDef.getChildrenAndExtension(); + final List result = new ArrayList<>(children.size()); - myHasNext = true; - return true; - } + for(final BaseRuntimeChildDefinition child: children) { + CompositeChildElement myNext = new CompositeChildElement(theParent, child, theEncodeContext); - @Override - public CompositeChildElement next() { - if (myHasNext == null) { - if (!hasNext()) { - throw new IllegalStateException(); - } - } - CompositeChildElement retVal = myNext; - myNext = null; - myHasNext = null; - return retVal; + /* + * There are lots of reasons we might skip encoding a particular child + */ + if (myNext.getDef().getElementName().equals("id")) { + continue; + } else if (!myNext.shouldBeEncoded(theContainedResource)) { + continue; + } else if (myNext.getDef() instanceof RuntimeChildNarrativeDefinition) { + if (isSuppressNarratives() || isSummaryMode()) { + continue; + } else if (theContainedResource) { + continue; } - - @Override - public void remove() { - throw new UnsupportedOperationException(); + } else if (myNext.getDef() instanceof RuntimeChildContainedResources) { + if (theContainedResource) { + continue; } - }; + } + result.add(myNext); } - }; + return result; + }); } private void containResourcesForEncoding(ContainedResources theContained, IBaseResource theResource, IBaseResource theTarget) { @@ -1274,6 +1267,37 @@ public boolean shouldBeEncoded(boolean theContainedResource) { return retVal; } + + @Override + public int hashCode() { + final int prime = 31; + int result = 1; + result = prime * result + ((myDef == null) ? 0 : myDef.hashCode()); + result = prime * result + ((myParent == null) ? 0 : myParent.hashCode()); + result = prime * result + ((myResDef == null) ? 0 : myResDef.hashCode()); + result = prime * result + ((myEncodeContext == null) ? 0 : myEncodeContext.hashCode()); + return result; + } + + @Override + public boolean equals(Object obj) { + if (this == obj) + return true; + + if (obj instanceof CompositeChildElement) { + final CompositeChildElement that = (CompositeChildElement) obj; + return Objects.equals(this.getEnclosingInstance(), that.getEnclosingInstance()) && + Objects.equals(this.myDef, that.myDef) && + Objects.equals(this.myParent, that.myParent) && + Objects.equals(this.myResDef, that.myResDef) && + Objects.equals(this.myEncodeContext, that.myEncodeContext); + } + return false; + } + + private BaseParser getEnclosingInstance() { + return BaseParser.this; + } } protected class EncodeContextPath { From cc3f02a895bd6fc6c763d48d5252434719ef9a33 Mon Sep 17 00:00:00 2001 From: David Maplesden Date: Tue, 17 Sep 2019 09:48:34 +1200 Subject: [PATCH 3/7] Optimise for encoding String primitive data types We typically have many more string primitives than anything else --- .../main/java/ca/uhn/fhir/parser/JsonParser.java | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/JsonParser.java b/hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/JsonParser.java index d391be8c3df3..de10c37a735d 100644 --- a/hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/JsonParser.java +++ b/hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/JsonParser.java @@ -224,13 +224,24 @@ private void encodeChildElementToStreamWriter(RuntimeResourceDefinition theResDe } case PRIMITIVE_DATATYPE: { final IPrimitiveType value = (IPrimitiveType) theNextValue; - if (isBlank(value.getValueAsString())) { + final String valueStr = value.getValueAsString(); + if (isBlank(valueStr)) { if (theForceEmpty) { theEventWriter.writeNull(); } break; } + // check for the common case first - String value types + if (value.getValue() instanceof String) { + if (theChildName != null) { + theEventWriter.write(theChildName, valueStr); + } else { + theEventWriter.write(valueStr); + } + break; + } + if (value instanceof IBaseIntegerDatatype) { if (theChildName != null) { write(theEventWriter, theChildName, ((IBaseIntegerDatatype) value).getValue()); @@ -262,7 +273,6 @@ public String toString() { } } } else { - String valueStr = value.getValueAsString(); if (theChildName != null) { write(theEventWriter, theChildName, valueStr); } else { From 36f53e64cbc687de0355784c0453027e70f543ef Mon Sep 17 00:00:00 2001 From: David Maplesden Date: Tue, 17 Sep 2019 12:28:37 +1200 Subject: [PATCH 4/7] We only need to extract the existing contained resources once --- .../java/ca/uhn/fhir/parser/BaseParser.java | 50 +++++++++---------- 1 file changed, 25 insertions(+), 25 deletions(-) diff --git a/hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/BaseParser.java b/hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/BaseParser.java index c9d433a89b8a..92b6ca026887 100644 --- a/hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/BaseParser.java +++ b/hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/BaseParser.java @@ -204,31 +204,6 @@ protected Iterable compositeChildIterator(IBase theCompos } private void containResourcesForEncoding(ContainedResources theContained, IBaseResource theResource, IBaseResource theTarget) { - - if (theTarget instanceof IResource) { - List containedResources = ((IResource) theTarget).getContained().getContainedResources(); - for (IResource next : containedResources) { - String nextId = next.getId().getValue(); - if (StringUtils.isNotBlank(nextId)) { - if (!nextId.startsWith("#")) { - nextId = '#' + nextId; - } - theContained.getExistingIdToContainedResource().put(nextId, next); - } - } - } else if (theTarget instanceof IDomainResource) { - List containedResources = ((IDomainResource) theTarget).getContained(); - for (IAnyResource next : containedResources) { - String nextId = next.getIdElement().getValue(); - if (StringUtils.isNotBlank(nextId)) { - if (!nextId.startsWith("#")) { - nextId = '#' + nextId; - } - theContained.getExistingIdToContainedResource().put(nextId, next); - } - } - } - List allReferences = getAllBaseReferences(theResource); for (IBaseReference next : allReferences) { IBaseResource resource = next.getResource(); @@ -268,6 +243,31 @@ private void containResourcesForEncoding(ContainedResources theContained, IBaseR protected void containResourcesForEncoding(IBaseResource theResource) { ContainedResources contained = new ContainedResources(); + + if (theResource instanceof IResource) { + List containedResources = ((IResource) theResource).getContained().getContainedResources(); + for (IResource next : containedResources) { + String nextId = next.getId().getValue(); + if (StringUtils.isNotBlank(nextId)) { + if (!nextId.startsWith("#")) { + nextId = '#' + nextId; + } + contained.getExistingIdToContainedResource().put(nextId, next); + } + } + } else if (theResource instanceof IDomainResource) { + List containedResources = ((IDomainResource) theResource).getContained(); + for (IAnyResource next : containedResources) { + String nextId = next.getIdElement().getValue(); + if (StringUtils.isNotBlank(nextId)) { + if (!nextId.startsWith("#")) { + nextId = '#' + nextId; + } + contained.getExistingIdToContainedResource().put(nextId, next); + } + } + } + containResourcesForEncoding(contained, theResource, theResource); contained.assignIdsToContainedResources(); myContainedResources = contained; From 8fe3fe789b58ced888628eb8ab63fca41ef2c9e3 Mon Sep 17 00:00:00 2001 From: David Maplesden Date: Tue, 17 Sep 2019 12:30:39 +1200 Subject: [PATCH 5/7] Cut short traversal when finding top-level base references --- hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/BaseParser.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/BaseParser.java b/hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/BaseParser.java index 92b6ca026887..cbd2948b59e9 100644 --- a/hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/BaseParser.java +++ b/hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/BaseParser.java @@ -287,6 +287,8 @@ protected List getAllBaseReferences(IBaseResource theResource){ protected void findBaseReferences(List allElements, IBase theElement, BaseRuntimeElementDefinition theDefinition) { if (theElement instanceof IBaseReference) { allElements.add((IBaseReference) theElement); + // end traversal here, we recursively process nested resources later + return; } BaseRuntimeElementDefinition def = theDefinition; From 6423cb23cef0f4ef22a3a3decda44dc0994723d3 Mon Sep 17 00:00:00 2001 From: David Maplesden Date: Tue, 17 Sep 2019 12:52:31 +1200 Subject: [PATCH 6/7] Add modifier extensions to tha actual modifier extensions collection --- hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/JsonParser.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/JsonParser.java b/hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/JsonParser.java index de10c37a735d..ebb6ef016b0e 100644 --- a/hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/JsonParser.java +++ b/hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/JsonParser.java @@ -469,7 +469,7 @@ private void encodeCompositeElementChildrenToStreamWriter(RuntimeResourceDefinit if (nextValue instanceof IBaseHasModifierExtensions) { IBaseHasModifierExtensions element = (IBaseHasModifierExtensions) nextValue; List> ext = element.getModifierExtension(); - force |= addToHeldExtensions(valueIdx, ext, extensions, true, nextChildElem, theParent, theEncodeContext, theContainedResource, theElement); + force |= addToHeldExtensions(valueIdx, ext, modifierExtensions, true, nextChildElem, theParent, theEncodeContext, theContainedResource, theElement); } } if (nextValue.hasFormatComment()) { From c0f85b571aea533b0ab1af8a88acb170dab2e4b3 Mon Sep 17 00:00:00 2001 From: David Maplesden Date: Tue, 17 Sep 2019 15:40:46 +1200 Subject: [PATCH 7/7] We can't short circuit traversal after all. --- hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/BaseParser.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/BaseParser.java b/hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/BaseParser.java index cbd2948b59e9..92b6ca026887 100644 --- a/hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/BaseParser.java +++ b/hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/BaseParser.java @@ -287,8 +287,6 @@ protected List getAllBaseReferences(IBaseResource theResource){ protected void findBaseReferences(List allElements, IBase theElement, BaseRuntimeElementDefinition theDefinition) { if (theElement instanceof IBaseReference) { allElements.add((IBaseReference) theElement); - // end traversal here, we recursively process nested resources later - return; } BaseRuntimeElementDefinition def = theDefinition;