Replace use of GetProperties with generated property getters in FlowAnalysis, and BusLoadAnalysis

[AaronGreenhouse]

Existing analysis code should be updated to use the generated property set accessor methods in org.osate.aadl2.contrib and now org.osate.contributions.sei

Replace the use of GetProperties in the bus load analysis and flow analysis to find out what issues exist with replacing uses of GetProperties with the generated accessor methods:

• What features are missing?
• What helper methods do we need?
• What makes replacements hard?
• What patterns are there?
• etc.

[AaronGreenhouse]

Not sure why this was closed.

[AaronGreenhouse]

Working on org.osate.contribution.sei uses.

• Marked class org.osate.importer.properties.CriticalityProperty as deprecated
• Marked method org.osate.analysis.flows.internal.utils.FlowLatencyUtil.getDimension() as deprecated
• Marked class org.osate.importer.properties.InspectProperty as deprecated
• Marked class org.osate.analysis.architecture.handlers.MissRateProperties as deprecated
• Marked class org.osate.importer.properties.SlocProperty as deprecated

[AaronGreenhouse]

Had to update org.osate.contribution.sei to reexport org.osate.pluginsupport, and org.osate.aadl2.contrib.

Updated FlowLatencyAnalysisSwitch to use Sei.getResponseTime(). Did a crude replacement. After updating the use of properties from the other property sets the code needs to further improved to take advantage of what is offered by using the new methods. Should replace some of the goofy logic used to handle properties in flow latency.

All is not well, however. Discovered that the generated property set code doesn't nicely handle the case where the property doesn't to apply to the AADL element. The underlying property lookup code throws a PropertyDoesNotApplyToHolderException. This is allowed to propagate to the caller. This would be fine if there were an easy way to test if the property applies, but there isn't one. You really just have to try it and see what happens. This exception should not be propagated, but caught internally to take advantage of the Optional class. Added this as issue #2593.

Update

@lwrage doesn't think the change proposed in issue #2593 is a good idea. Better to be smarter about looking properties. Need to more fully review how to update the use of properties in FlowLatencyAnalysisSwitch before making changes to other code.

[AaronGreenhouse]

Updated org.osate.aadl2.contrib to reexport org.osate.pluginsupport for the types in org.osate.pluginsupport.properties.

[AaronGreenhouse]

Two uses of the old TimingProperties in AadlBaLegalityRulesChecker. One use is easy to replace, but the other is strange because it gets the value without regard to units and compares it to another value.

[AaronGreenhouse]

I've decided to focus on the BusLoadAnalysis for the moment because I actually understand what that is intending to do with properties. I am wondering what we want to do with methods in GetProperties that perform complex operations, like getDataSize():

    public static double getDataSize(final NamedElement ne, UnitLiteral unit) {
        Property DataSize = lookupPropertyDefinition(ne, MemoryProperties._NAME, MemoryProperties.DATA_SIZE);
        Property SourceDataSize = lookupPropertyDefinition(ne, MemoryProperties._NAME,
                MemoryProperties.SOURCE_DATA_SIZE);
        double res = PropertyUtils.getScaledNumberValue(ne, DataSize, unit, 0.0);
        if (res == 0.0) {
            res = PropertyUtils.getScaledNumberValue(ne, SourceDataSize, unit, 0.0);
        }
        long mult = 1;
        if (ne instanceof DataSubcomponent) {
            mult = AadlUtil.getMultiplicity(ne);
        }
        if (res != 0.0) {
            return res * mult;
        }
        if (ne instanceof DataSubcomponent || ne instanceof DataImplementation) {
            // mult is one or the array size of the data subcomponent.
            return sumElementsDataSize(ne, unit, DataSize, SourceDataSize, 0) * mult;
        }
        return 0.0;
    }

This method is obviously doing something at a higher semantic level than just getting a property value. It probably belongs somewhere else, but where?

[AaronGreenhouse]

The following method seems useful, but I'm not sure where to put it:

    private static <U extends Enum<U> & GeneratedUnits<U>> Optional<Double> getScaled(
            final Function<NamedElement, Optional<? extends Scalable<U>>> f, final NamedElement ne,
            final U unit) {
        return f.apply(ne).map(rwu -> rwu.getValue(unit));
    }

I had to add a new Scalable interface to package org.osate.pluginsupport.properties:

public interface Scalable<U extends Enum<U> & GeneratedUnits<U>> {
    public double getValue(U targetUnit);
}

This is implemented by IntegerWithUnits and RealWIthUnits.

[AaronGreenhouse]

Updated NewBusLoadAnalysis and BusLoadModelBuilder to not use GetProperties.

[AaronGreenhouse]

Seeing what can be done with methods in InstanceModelUtil

[AaronGreenhouse]

Updated InstanceModelUtil to not use GetProperties. Had to make alternative versions of methods

• getFunctionBinding
• getMemoryBinding
• getProcessorBinding
• getConnectionBinding

because they are declared to return List<ComponentInstance> but using the new getter methods returns List<InstanceObject>. The new versions that return List<InstanceObject> are

• getFunctionBindings
• getMemoryBindings
• getProcessorBindings
• getConnectionBindings

The original methods are marked as @deprecated. They have been updated to use the new getter methods and not use GetProperties, but in order to work around the typing issue, they must recopy a list internally.

[AaronGreenhouse]

Changes affected the flow latency analysis because using the new getter methods causes exceptions when the property doesn't apply to something. I had to add some guards to methods in InstanceModelUtil, and fix a call site in FlowLatencyAnalysisSwitch to prevent these.

[AaronGreenhouse]

Need to fix

• FlowLatencyAnalaysisSwitch.hasResponseTime()
• FlowLatencyAnalaysisSwitch.getScaledMaxResponseTimeinMilliSec()
• FlowLatencyAnalaysisSwitch.getScaledMinResponseTimeinMilliSec()

That will take care of the all the uses of old classes from org.osate.contribution.sei

[AaronGreenhouse]

Most of the existing code assumes "magic" when a property value is not present, or if the property does not apply to given model element. This magic is usually the return of a default value that depends on the context. Fixing this properly involves making changes to the call sites that use properties to avoid passing in elements to which the property does not apply. Missing property values are more easily dealt with because they return an "empty" Optional object.

[AaronGreenhouse]

I keep forgetting that abstract components and features can have any property association. This affects the checks I have to make to guard property lookups.

• abstract components and subcomponents may have any property
• abstract features are not treated specially

This distinction is a bit confusing.

[AaronGreenhouse]

General comment: generated getter methods lend themselves to functional programming using lambda expressions and method handles. This is mostly due to the use of the Optional class.

[AaronGreenhouse]

Some helper methods I've deposited that I need to find a better home for if they are generally useful:

public class InstanceModelUtil {

    /* XXX: Where to put this? */
    private static final <V> boolean propertyEquals(final Function<NamedElement, Optional<V>> f, final NamedElement ne,
            final V value) {
        return f.apply(ne).map(x -> x == value).orElse(false);
    }

    /* XXX: WHere to put this? */
    private static final List<ComponentInstance> getListAsComponentInstance(
            final Function<NamedElement, Optional<List<InstanceObject>>> f, final NamedElement ne) {
        return f.apply(ne).map(v -> {
            final List<ComponentInstance> list = new ArrayList<>(v.size());
            v.forEach(x -> list.add((ComponentInstance) x));
            return list;
        }).orElse(Collections.emptyList());
    }

from NewBusLoadAnalysis

    /* XXX: Where to put this? */
    private static <U extends Enum<U> & GeneratedUnits<U>> Optional<Double> getScaled(
            final Function<NamedElement, Optional<? extends Scalable<U>>> f, final NamedElement ne,
            final U unit) {
        return f.apply(ne).map(rwu -> rwu.getValue(unit));
    }

From FlowLatencyAnalysisSwitch

    /* XXX: Where to put this? */
    private static <U extends Enum<U> & GeneratedUnits<U>> Optional<Double> getScaled(
            final Function<NamedElement, Optional<IntegerRangeWithUnits<U>>> f, final NamedElement ne,
            final Function<IntegerRangeWithUnits<U>, IntegerWithUnits<U>> f2, final U unit) {
        return f.apply(ne).map(rrwu -> f2.apply(rrwu).getValue(unit));
    }

[AaronGreenhouse]

Going through the rest of flow analysis to get rid of reliance of GetProperties

[AaronGreenhouse]

Finished updating the flow analysis.

Left behind a class AnalysisUtils in org.osate.analysis.flows.internal.utils that contains rewritten methods from GetProperties that seem to be more widely used. Not sure where to but them at the moment.

[AaronGreenhouse]

Problem:

FlowLatencyAnalysisSwitch.mapComponentInstance() directly tries to test if a property value is present using statements such as

        boolean isAssignedDeadline = GetProperties.isAssignedDeadline(componentInstance);

and

                    if ((InstanceModelUtil.isThread(componentInstance) || InstanceModelUtil.isDevice(componentInstance))
                            && !GetProperties.hasAssignedPropertyValue(componentInstance, "Dispatch_Protocol")) {
                        samplingLatencyContributor.reportInfo("Assume Periodic dispatch because period is set");

these ultimately use GetProperties.isAssignedPropertyValue():

    public static boolean isAssignedPropertyValue(NamedElement element, Property pn) {
        try {
            final PropertyAcc propertyAccumulator = element.getPropertyValue(pn);
            PropertyAssociation firstAssociation = propertyAccumulator.first();
            return firstAssociation != null;
        } catch (org.osate.aadl2.properties.PropertyDoesNotApplyToHolderException exception) {
            return false;
        }

    }

I tried to replace the above uses as follows:

        boolean isAssignedDeadline = TimingProperties.getDeadline(componentInstance).map(x -> true).orElse(false);

and

                    if ((InstanceModelUtil.isThread(componentInstance) || InstanceModelUtil.isDevice(componentInstance))
                            && ThreadProperties.getDispatchProtocol(componentInstance).map(x -> false).orElse(true)) {
                        samplingLatencyContributor.reportInfo("Assume Periodic dispatch because period is set");

This is not the same somehow. Existing regression tests break when I do this. I'm not sure why. I rolled back this change for now because I have other failed regression tests I need to fix.

[AaronGreenhouse]

The following replacement also didn't work correctly (also in FlowLatencyAnalysisSwitch.mapComponentInstance()):

            final OptionalLong queueSize =
                    (incomingConnectionFI.getCategory() == FeatureCategory.EVENT_PORT
                            || incomingConnectionFI.getCategory() == FeatureCategory.EVENT_PORT
                            || incomingConnectionFI.getCategory() == FeatureCategory.SUBPROGRAM_ACCESS)
                                    ?
                    org.osate.aadl2.contrib.communication.CommunicationProperties
                    .getQueueSize(incomingConnectionFI) : OptionalLong.empty();
            if (queueSize.isPresent()) {
                qs = queueSize.getAsLong();

for

            if (GetProperties.hasAssignedPropertyValue(incomingConnectionFI, CommunicationProperties.QUEUE_SIZE)) {
                qs = GetProperties.getQueueSize(incomingConnectionFI);

Undid for now.

[AaronGreenhouse]

Fixed the above problems by copying the isAssigned methods from GetProperties into the class as private methods.

Flow analysis no longer depends on GetProperties.

[AaronGreenhouse]

Updating the use in Aadl2Validator. Most cases were straightforward, but I found some cases where the value of a property constant is needed, and this is not supported by the code generator. Opened issue #2632 for this.

[AaronGreenhouse]

Updated all of Aadl2Validator except for the property constants.

Going to stop updating things at this point. Will document on a wiki page my experience and describe the helper methods I added.

[AaronGreenhouse]

Good place to stop for now.

Renaming this issue "Explore replacing use of GetProperties with generated property methods"

[AaronGreenhouse]

Added wiki page that discusses this all in a cleaner manner

https://github.com/osate/osate2/wiki/Using-Generated-AADL-Property-Getter-Methods