You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
The classes in osmium::index::map/multimap are just simple wrapper classes for different data structures. Most of their functions are really simple and good candidates for inlining. This would work if thoses classes are simple template classes. But in some cases we also want to use them polymorphically, ie we want to decide at runtime which implementation to use depending on the kind of OSM data we work with (mainly whole planet vs. small extract) and how much memory we have available. For that they have to be virtual derived classes.
This becomes worse for the multimap classes, because their get_all() functions returns two iterators through which we can access the results. But the type of those iterators is different depending on the implementation of the multimap class. To make this work polymorphically, the iterators would have to be polymorphic, too. Of course all of this slows down the use case where you already know at compile time what class you want.
The currently situation is a strange mix. The multimap classes are virtual derived classes, but some of their functions (for instance get_all()) are not virtual and they have different return types in the different derived classes. This is not only ugly, it is error prone and hard to work with and will not work polymorphically.
One option would be to split the functionality into one class hierarchy that can not be used polymorphically (so you have to decide at compile time) and another class hierarchy on top of that that is usable polymorphically.
And maybe all these complications are not really problematic, because the cost of not inlining but having those virtual function calls isn't all that great compared to what else Osmium is doing. Maybe we should benchmark that... Unfortunately we have to implement both variants to do that...
To decide on the right design, we might have to look at more use cases.
The text was updated successfully, but these errors were encountered:
The classes in osmium::index::map/multimap are just simple wrapper classes for different data structures. Most of their functions are really simple and good candidates for inlining. This would work if thoses classes are simple template classes. But in some cases we also want to use them polymorphically, ie we want to decide at runtime which implementation to use depending on the kind of OSM data we work with (mainly whole planet vs. small extract) and how much memory we have available. For that they have to be virtual derived classes.
This becomes worse for the multimap classes, because their get_all() functions returns two iterators through which we can access the results. But the type of those iterators is different depending on the implementation of the multimap class. To make this work polymorphically, the iterators would have to be polymorphic, too. Of course all of this slows down the use case where you already know at compile time what class you want.
The currently situation is a strange mix. The multimap classes are virtual derived classes, but some of their functions (for instance get_all()) are not virtual and they have different return types in the different derived classes. This is not only ugly, it is error prone and hard to work with and will not work polymorphically.
One option would be to split the functionality into one class hierarchy that can not be used polymorphically (so you have to decide at compile time) and another class hierarchy on top of that that is usable polymorphically.
And maybe all these complications are not really problematic, because the cost of not inlining but having those virtual function calls isn't all that great compared to what else Osmium is doing. Maybe we should benchmark that... Unfortunately we have to implement both variants to do that...
To decide on the right design, we might have to look at more use cases.
The text was updated successfully, but these errors were encountered: