Skip to content
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.

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

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

Closed
AaronGreenhouse opened this issue Feb 11, 2021 · 25 comments · Fixed by #2639
Assignees
Labels
Milestone

Comments

@AaronGreenhouse
Copy link
Contributor

AaronGreenhouse commented Feb 11, 2021

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
Copy link
Contributor Author

Not sure why this was closed.

@AaronGreenhouse AaronGreenhouse self-assigned this Feb 23, 2021
@AaronGreenhouse
Copy link
Contributor Author

AaronGreenhouse commented Feb 24, 2021

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
Copy link
Contributor Author

AaronGreenhouse commented Feb 24, 2021

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
Copy link
Contributor Author

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

@AaronGreenhouse
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

Updated NewBusLoadAnalysis and BusLoadModelBuilder to not use GetProperties.

@AaronGreenhouse
Copy link
Contributor Author

Seeing what can be done with methods in InstanceModelUtil

@AaronGreenhouse
Copy link
Contributor Author

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
Copy link
Contributor Author

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.

@lwrage lwrage added analyses analyses from plugins category:enhancement labels Mar 11, 2021
@AaronGreenhouse
Copy link
Contributor Author

AaronGreenhouse commented Mar 11, 2021

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@AaronGreenhouse
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

AaronGreenhouse commented Apr 5, 2021

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

Good place to stop for now.

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

@AaronGreenhouse AaronGreenhouse changed the title Update analysis code to use generated property set methods Explore replacing use of GetProperties with generated property methods Apr 8, 2021
@AaronGreenhouse AaronGreenhouse changed the title Explore replacing use of GetProperties with generated property methods Replace use of GetProperties with generated property getters in FlowAnalysis, and BusLoadAnalysis Apr 9, 2021
@AaronGreenhouse
Copy link
Contributor Author

AaronGreenhouse commented Apr 27, 2021

Added wiki page that discusses this all in a cleaner manner

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

@lwrage lwrage added this to the 2.10.0 milestone Apr 28, 2021
@lwrage lwrage removed this from the 2.10.0 milestone Jul 2, 2021
@lwrage lwrage added this to the 2.10.0 milestone Aug 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants