tud-zih-energy / otf2xx

C++ wrapper for Open Trace Format 2
BSD 3-Clause "New" or "Revised" License
9 stars 5 forks source link

Usage of io_file #27

Closed Flamefire closed 6 years ago

Flamefire commented 6 years ago

io_file is not a real definition. It is rather an interface. As such there cannot be an instance of io_file but only of one of its subclasses io_regular_file and io_directory. This is similar to metric being the interface to metric_class/metric_instance. The difference is that the io classes happen to have the same members which is why the current code works e.g. io_handle

But this breaks as soon as another member is added to either class. And it is broken for the new registry Having 3 containers is a bug. Either there should be one for all io_files or 2 for the subclasses.

I can't think of a good general solution ATM. We either need references/pointers to the base type or e.g variants. Or go with the hack in event::metric where 2 instances are stored but only 1 is valid and never use the base type.

Until then: Remove the io_file container or the other 2 from the registry.

bmario commented 6 years ago

metrics are different. Fixed the issue in the registry. io_files are still constructable, but you can't write them using the writer interface and you won't get one from the reader.

Flamefire commented 6 years ago

Can you explain why metrics are different?

From the OTF2 specification:

C.28 OTF2_MetricRef Metric This is a polymorphic definition class Derivations

  • MetricClass
  • MetricInstance

C.47 OTF2_IoFileRef IoFile This is a polymorphic definition class. Derivations

  • IoRegularFile
  • IoDirectory

I can't see any difference there. So the only difference I see is that IoRegularFile and IoDirectory happen to have the same members which is a coincidence only from a technical point of view.

tilsche commented 6 years ago

MetricClass and MetricInstrance were made "polymorphic" in retrospect after this concept was introduced for IoFile. It doesn't work very well for Metric, e.g. the metricClass member of a MetricInstance must always be a MetricClass despite officially referring to a Metric. So using an actual reference to MetricClass rather than Metric gives better static type safety for the definition.

Technically MetricClass and MetricInstance have no members in common - but from a logical point of view MetricInstance should be derived from MetricClass - but that doesn't work at all because you need two separate ids.

Flamefire commented 6 years ago

but from a logical point of view MetricInstance should be derived from MetricClass

From my understanding of the wording polymorphic and Derivations I'd expect Metric and IoFile to be interfaces and the others derived from that interface. From the above definition and the description in the OTF2 manual I do not see, why one derived class must be derived from another. Although they could as long the parent derives from the "interface". This might make sense (Only) for the IoRegularFile/IoDirectory although it violates OOP principles (liskovsche substitution IIRC)

but that doesn't work at all because you need two separate ids.

I don't understand this. Both definitions share the id space, and every actual instance needs a separate id, which is true for all definition instances.

tilsche commented 6 years ago

From my understanding of the wording polymorphic and Derivations I'd expect Metric and IoFile to be interfaces and the others derived from that interface.

Yes, but apart from using the same ID space, they technically have nothing in common. A naked Metric base class would be useless.

My point is - you can always get a MetricClass from any of the two kinds of Metrics - but that is not reflected by the current polymorphism.

Flamefire commented 6 years ago

A naked Metric base class would be useless.

That is why I called it an interface. From a MetricRef you'll always get a Metric. Whether it is a MetricClass or MetricInstance is defined somehere else. Same for IoFile.

My point is - you can always get a MetricClass from any of the two kinds of Metrics - but that is not reflected by the current polymorphism.

A MetricClass is-a MetricClass but a MetricInstance has-a MetricClass. And for both hold is-a Metric. That is different. Where does the MetricInstance is-a MetricClass come from?

Speaking in C++: Why should you be able to do a dynamic_cast<MetricClass*>(metricInstance)? This would be like casting the number 5 to the type int which is different to casting 5 to an instance of int.

bmario commented 6 years ago

Of what worth is an empty interface? And technically MetricClass and MetricInstance has a MetricRef. Anyways, I don't see were we are going in this discussion.

Flamefire commented 6 years ago

Of what worth is an empty interface?

To do what is technically valid by the OTF2 spec (Made up example, but shows the point):

Metric* m1 = new MetricClass;
Metric* m2 = new MetricInstance;
// Or
Metric* get(MetricRef ref){...}

And technically MetricClass and MetricInstance has a MetricRef.

Only in otf2xx where this is an implementation detail/choice. This is ok during reading, but has a downside during writing: You need to get a MetricRef first before being able to create a MetricClass/MetricInstance while it should be the other way round (how can there be a reference when the referenced object does not exist yet?). Doesn't matter though, just wanted to point out that this is otf2xx not OTF2.
So technically the super class could contain the ref function returning MetricRef in this case and it would be reasonable.

Anyways, I don't see were we are going in this discussion.

Metric and Io_File are similar in terms of OTF2. There should never be an instance of any of them but only of one of its subclasses -> Interface