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

Use list index instead of value reference to identify variables #518

Closed
t-sommer opened this Issue Jan 10, 2019 · 6 comments

Comments

Projects
None yet
4 participants
@t-sommer
Copy link
Collaborator

t-sommer commented Jan 10, 2019

According to Getting and Setting Variable Values the attribute valueReference must be unique for all Variables in FMI 3.0:

Element valueReference shall be unique for all variables.

However each Variable is already uniquely identified by its list index. I suggest to remove the valueReference attribute and use the list index instead.

In order to leave the C API untouched we could implicitly assume that the value reference equals the 1-based list index of the Variable:

<ModelVariables>
    <Variable name="a" valueReference="1">...</Variable>
    <Variable name="b" valueReference="2">...</Variable>
    <Variable name="c" valueReference="3">...</Variable>
</ModelVariables>

@t-sommer t-sommer added this to the FMI3.0 milestone Jan 10, 2019

@t-sommer t-sommer added the discussion label Jan 10, 2019

@andreas-junghanns

This comment has been minimized.

Copy link
Contributor

andreas-junghanns commented Jan 10, 2019

We use a valueReference integer that we can use internally to quickly access the data. Using a list index would require us to use one more indirection for access. Therefore, I am not a huge fan of this proposal.

@t-sommer

This comment has been minimized.

Copy link
Collaborator

t-sommer commented Jan 10, 2019

@andreas-junghanns, I assume that your implementation checks the VR for validity (instead of blindly casting it to a memory address). In the same step you could efficiently map it to your internal memory layout. For the example above this could look like:

struct VariableInfo {
    VariableType type;
    void *memoryAddress;
};

VariableInfo vrs[] = { 
   {Float64, &myDoubles[0]},
   {Int32,   &myInts[3]},
   {Boolean, &myBools[15]},
};

// 1-based variable index
unsigned int index = 2;

// get address for index 2
void *pointerToB = vrs[index-1].memoryAddress;
@pmai

This comment has been minimized.

Copy link
Collaborator

pmai commented Jan 10, 2019

@t-sommer this is rather costly, since it will cause a memory access, which depending on environment, number of FMUs, number of variables is likely not to be in L1 Cache, so this is potentially an order of magnitude (or more) slower than simply having defined ranges, which just require two compares and an addition to check the VR and generate the index (plus potentially a shift to generate the address).

I also don't see the benefit in doing this switch: We have gone to great lengths to get rid of the stupid 1-based XML variable index. If we now reintroduce it, we get all of the bad stuff we had (i.e. brittle XML, need to rewrite everything if one thing changes, etc.), but now we also have no ability at all to modify the XML after generation, because this will change the API values: Nowadays tools can remove variables from the modelDescription.xml of an FMU, with a good probability that this will still work correctly (i.e. I have never seen an FMU in the wild where this does not work. While we warn people that modifying the XML is not guaranteed to work, it does work for IP-protection fairly well). With this change this is never going to work.

Weighing against this, I can see no benefit; maybe I'm missing something?

@t-sommer

This comment has been minimized.

Copy link
Collaborator

t-sommer commented Jan 10, 2019

The obvious benefit would be to simplify the XML by moving the value references to the implementation.

@pmai

This comment has been minimized.

Copy link
Collaborator

pmai commented Jan 10, 2019

It does not simplify it, really, it just replaces an obvious index (the VR) with a non-obvious one, namely the position of the child element in document order, which arguably, since it is implicit rather than explicit, makes the XML more complex, not less complex.

@chrbertsch

This comment has been minimized.

Copy link
Collaborator

chrbertsch commented Jan 11, 2019

Discussion at meeting 11.1.2019:
Pierre: Mai: Do not like this proposal on its own. (less flexibility, e.g., to remove variables). Might make sense within a bigger change removing the set/get functions (gaining, e.g., performance; but loosing flexibility)
Karl: I do not see the point
Torsten S: The simplifies the XML
Kaska: How does it affect alias variables
Torsten S: Alias variables must be unique
Pierre: This must be clarified
Andreas J: We decided to get away with alias variables
Karl, Pierre: Do not remember this decision
Andreas J: We decided to have value references unique across all types.
Pierre: This would still allow different variables to have the same VR.
Pierre: We decided to restrict the usage of aliases, but not to remove them.
Karl: Has the section on aliases to be removed?
Andreas J: Only a sentence on uniqueness across types has been added.
Pierre: I do a Pull request to clarify this
Torsten S: Should be discussed in a larger context, which would take more time than for the FMI 3.0 release. (e.g. an additional, optional shared memory interface instead of the getters/ setters; could be done in a minor release. Could also be a layered standard)
--> close for now, create a new ticket for the larger change (not for FMI 3, Pierre)

@chrbertsch chrbertsch closed this Jan 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment