Topics

Multiple sources of SystemName comparisons

Randall Wood
 

While working on [#7660](https://github.com/JMRI/JMRI/issues/7660), I found that [#4669](https://github.com/JMRI/JMRI/pull/4669) (specifically [this commit](https://github.com/JMRI/JMRI/commit/8cf33bbb2fcf2b9bbc9f770ee3e26ad4d81077f4)) introduced the incorrect sorting rules for Internal NamedBeans, but that [#7278](https://github.com/JMRI/JMRI/pull/7278) is the first time JMRI relied on that incorrect sorting rule.

This reveals that AbstractManager’s reliance on three sets of data to store NamedBeans (_beans - a SortedTreeSet of NamedBeans, _sys - a HashMap of NamedBeans by SystemName, and _user - a HashMap of NamedBeans by UserName) means that AbstractManager has been internally inconsistent.

I’m surprised the incorrect sorting in #4669 managed to go unnoticed as long as it has (or more likely was never recognized as the cause of these problems).

#7666 addresses the problem in the Managers by having the Managers get the comparator to use from the associated SystemConnectionMemo *and* by updating some classes to override `compareSystemNameSuffix(String, String, NamedBean)` correctly.

The question is: since the ground truth of what a valid system name is maintained separately from the ground truth of how two system names should be compared (and its likely that both of these truths are also spread across multiple classes), should this be consolidated into a single place that provides a per-system connection source of that truth?

Randall

Bob Jacobsen
 

Some thoughts in response:

Not quite sure what "its likely that both of these truths are also spread across multiple classes” means; the system name architecture means that (once we have always-parseable system names in 4.19) the sort is first on connection name, and only if that's the same system connection is it needed to invoke system-specific parsing; there’s never a need to do two systems worth of system-specific parsing/sorting, as that would become an NxN problem in the code. Perhaps there’s some problem here I don’t understand.

SystemConnectionMemo is an jmri.jmrix construct. It’s an architecture level-jumping issue to have it (already) publicly present in jmri.Manager, and also used by implementations in jmri.NamedBeanHandleManager, jmri.BlockManager, and others. Perhaps some or all of it should be promoted to the jmri package, particularly if it’s going to get more use from non-jmri.jmrix interfaces and classes.

If you want to make sure that multiple functional items are consistently implemented, i.e. a comparator, a formatter and a validator, then one approach:
- Define an interface that provides all of those
- Create multiple implementations, each self-consistent, as needed; One AlphaNumeric, one PurelyAlpha, one with MERGFormatting, one for C/MRI etc.
- Have the classes that use these functional items take and hold an instance of that interface, then invoke that instance as needed for the behavior
- Install the appropriate implementation at construction time
Those instances are very lightweight (probably no data members at all), so there’s no need to use shared instances.

Such a common-function object could also be provided by the SystemConnectionMemo, although see above about access to that. Managers are usually constructed _from_ the SystemConnectionMemo, so it can provide the common-function object through the Manager constructor instead of being publicly accessible. But there might be value to having the sorting et al available more broadly than in just the Managers, i.e. to GUI code or scripts.

The internal redundant collections are to make getSystemNameArray(), getSystemNameList(), and getNamedBeanList() more performant. It would be nice to finish the migration away from those multiple retrieval interfaces in Manager to just the getSortedSet() one. Those methods have been deprecated since 4.11.5 (April 2017). For the JMRI code itself, only getSystemNameList() would take work to remove; the others are straight-forward. But CATS and other code, including probably quite a few user scripts, still depend on them. We could simplify the Manager implementation(s) by giving up that performance (perhaps in the process motivating migration to getSortedSet() ) and implementing each of the deprecated methods directly on top of getSortedSet() instead of having internal caches.

Bob




On Dec 1, 2019, at 7:47 AM, Randall Wood via Groups.Io <rhwood=mac.com@groups.io> wrote:

While working on [#7660](https://github.com/JMRI/JMRI/issues/7660), I found that [#4669](https://github.com/JMRI/JMRI/pull/4669) (specifically [this commit](https://github.com/JMRI/JMRI/commit/8cf33bbb2fcf2b9bbc9f770ee3e26ad4d81077f4)) introduced the incorrect sorting rules for Internal NamedBeans, but that [#7278](https://github.com/JMRI/JMRI/pull/7278) is the first time JMRI relied on that incorrect sorting rule.

This reveals that AbstractManager’s reliance on three sets of data to store NamedBeans (_beans - a SortedTreeSet of NamedBeans, _sys - a HashMap of NamedBeans by SystemName, and _user - a HashMap of NamedBeans by UserName) means that AbstractManager has been internally inconsistent.

I’m surprised the incorrect sorting in #4669 managed to go unnoticed as long as it has (or more likely was never recognized as the cause of these problems).

#7666 addresses the problem in the Managers by having the Managers get the comparator to use from the associated SystemConnectionMemo *and* by updating some classes to override `compareSystemNameSuffix(String, String, NamedBean)` correctly.

The question is: since the ground truth of what a valid system name is maintained separately from the ground truth of how two system names should be compared (and its likely that both of these truths are also spread across multiple classes), should this be consolidated into a single place that provides a per-system connection source of that truth?

Randall
--
Bob Jacobsen
@BobJacobsen

Randall Wood
 

On 01-Dec-2019, at 11:35, Bob Jacobsen <@BobJacobsen> wrote:

Some thoughts in response:

Not quite sure what "its likely that both of these truths are also spread across multiple classes” means; the system name architecture means that (once we have always-parseable system names in 4.19) the sort is first on connection name, and only if that's the same system connection is it needed to invoke system-specific parsing; there’s never a need to do two systems worth of system-specific parsing/sorting, as that would become an NxN problem in the code. Perhaps there’s some problem here I don’t understand.
This means that both NamedBeans and Managers maintain separate rules for what constitutes a valid system name for a specific class of NamedBean within a single connection, and we do not seem to have a reliably testable means of ensuring consistency between a NamedBean and its Manager.

SystemConnectionMemo is an jmri.jmrix construct. It’s an architecture level-jumping issue to have it (already) publicly present in jmri.Manager, and also used by implementations in jmri.NamedBeanHandleManager, jmri.BlockManager, and others. Perhaps some or all of it should be promoted to the jmri package, particularly if it’s going to get more use from non-jmri.jmrix interfaces and classes.

If you want to make sure that multiple functional items are consistently implemented, i.e. a comparator, a formatter and a validator, then one approach:
- Define an interface that provides all of those
- Create multiple implementations, each self-consistent, as needed; One AlphaNumeric, one PurelyAlpha, one with MERGFormatting, one for C/MRI etc.
- Have the classes that use these functional items take and hold an instance of that interface, then invoke that instance as needed for the behavior
- Install the appropriate implementation at construction time
Those instances are very lightweight (probably no data members at all), so there’s no need to use shared instances.

Such a common-function object could also be provided by the SystemConnectionMemo, although see above about access to that. Managers are usually constructed _from_ the SystemConnectionMemo, so it can provide the common-function object through the Manager constructor instead of being publicly accessible. But there might be value to having the sorting et al available more broadly than in just the Managers, i.e. to GUI code or scripts.

The internal redundant collections are to make getSystemNameArray(), getSystemNameList(), and getNamedBeanList() more performant. It would be nice to finish the migration away from those multiple retrieval interfaces in Manager to just the getSortedSet() one. Those methods have been deprecated since 4.11.5 (April 2017). For the JMRI code itself, only getSystemNameList() would take work to remove; the others are straight-forward. But CATS and other code, including probably quite a few user scripts, still depend on them. We could simplify the Manager implementation(s) by giving up that performance (perhaps in the process motivating migration to getSortedSet() ) and implementing each of the deprecated methods directly on top of getSortedSet() instead of having internal caches.

Bob




On Dec 1, 2019, at 7:47 AM, Randall Wood via Groups.Io <rhwood=mac.com@groups.io> wrote:

While working on [#7660](https://github.com/JMRI/JMRI/issues/7660), I found that [#4669](https://github.com/JMRI/JMRI/pull/4669) (specifically [this commit](https://github.com/JMRI/JMRI/commit/8cf33bbb2fcf2b9bbc9f770ee3e26ad4d81077f4)) introduced the incorrect sorting rules for Internal NamedBeans, but that [#7278](https://github.com/JMRI/JMRI/pull/7278) is the first time JMRI relied on that incorrect sorting rule.

This reveals that AbstractManager’s reliance on three sets of data to store NamedBeans (_beans - a SortedTreeSet of NamedBeans, _sys - a HashMap of NamedBeans by SystemName, and _user - a HashMap of NamedBeans by UserName) means that AbstractManager has been internally inconsistent.

I’m surprised the incorrect sorting in #4669 managed to go unnoticed as long as it has (or more likely was never recognized as the cause of these problems).

#7666 addresses the problem in the Managers by having the Managers get the comparator to use from the associated SystemConnectionMemo *and* by updating some classes to override `compareSystemNameSuffix(String, String, NamedBean)` correctly.

The question is: since the ground truth of what a valid system name is maintained separately from the ground truth of how two system names should be compared (and its likely that both of these truths are also spread across multiple classes), should this be consolidated into a single place that provides a per-system connection source of that truth?

Randall
--
Bob Jacobsen
@BobJacobsen