Details
-
Improvement
-
Status: Closed
-
Normal
-
Resolution: Fixed
-
None
-
None
-
None
-
Pulsar
-
Pulsar 239 -DynamicComponents4
Description
For CMS-13609 in org.hippoecm.hst.platform.configuration.components.HstComponentConfigurationIT#assertions_new_and_old_style_container_items I've added an assertion:
// TODO CMS-13618 the dynamic component parameters should be shared even for old style components assertThat(oldStyleBanner.getDynamicComponentParameters() == oldStyleBanner2.getDynamicComponentParameters()) .as("Old style components dynamic component parameters are not yet shared") .isFalse(); assertThat(oldStyleBanner.getDynamicComponentParameters() == newStyleBanner.getDynamicComponentParameters()) .as("Old style components dynamic component parameters are not yet shared") .isFalse();
The 'old style component items' which do not backreference a catalog item also load DynamicComponentParameters and FieldGroups, but IIUC, the only thing that impacts the DynamicComponentParameters is the java classname.
Given that, old style component items should share the DynamicComponentParameters instances just like the new style component items do: We have customers with tens of thousands of old style components, which now all load the DynamicComponentParameters and FieldGroups individually and all keep their own instances, which is a CPU and memory waste and might make the memory footprint quite a bit larger. Instead, we should share these instances
Once this is improved, the integration test expectations can flip from false to true for
assertThat(oldStyleBanner.getDynamicComponentParameters() == oldStyleBanner2.getDynamicComponentParameters())
.as("Old style components dynamic component parameters are not yet shared")
.isFalse();
assertThat(oldStyleBanner.getDynamicComponentParameters() == newStyleBanner.getDynamicComponentParameters())
.as("Old style components dynamic component parameters are not yet shared")
.isFalse();
Possible fix
In HstComponentsConfigurationService there is:
for (HstComponentConfiguration child : populate) { if (isEmpty(child.getComponentDefinition())) { // In case the component is a container item, this is legacy component instances support. For components // that are not hst:components, this is not legacy! // If component instance does not have a component definition reference, explicitly populate component parameters. //TODO SS: In case of legacy component instances, there is an extra memory overhead because // field group information & parameter definitions are stored on a component instance level, // instead of storing that on a catalog item level. // More over, field group info has no use during delivery and really belongs to a catalog item level // ((HstComponentConfigurationService) child).populateAnnotationComponentParameters(websiteClassLoader); ((HstComponentConfigurationService) child).populateFieldGroups(websiteClassLoader); ((HstComponentConfigurationService) child).populateComponentReferences(canonicalComponentConfigurations); } }
Adding to the populateAnnotationComponentParameters and populateFieldGroups a second parameter, something like Map<Class, List<DynamicParameter> hstDynamicComponentParameters> and Map<Class, List<DynamicFieldGroup>> which keeps a reference from class to a list and then in case the class is already loaded use the list from the map (and if not add it) should be a simple fix for this
Attachments
Issue Links
- relates to
-
CMS-13609 Create catalog integration tests for using catalog referencing support instead of prototype
-
- Closed
-