Add new feature 'full-metadata' to dump all available metadata from pbf file

[jocelynj]

Hi,

This commit adds a new config "full-metadata" to dump all Info fields from the PBF format, if they are available. I’ve kept them as Option<> to align with how the proto file is defined - it defines most of the fields as optional.

New available fields:

• version
• timestamp, as an i64 number, in milliseconds since 1970 epoch
• changeset
• user id
• user name
• visible, set to true by default

This is greatly inspired from PR ##40, with the addition of more fields.

I’ve launched some basic read tests using par_iter(), with/without the config on France pbf, which is 4.4GB big. To read all nodes/ways/relations, without any additional computation, on a 2*6-cores cpu, it takes:

• 18 s ± 0.2 s without metadata
• 24 s ± 2.9 s with metadata

I think it makes sense to keep the configurability, to not impact performance, but I can remove it if you prefer.

[TeXitoi]

Adding a field in a struct with all field public break the "adding a feature is not breaking" contract. For example, if someone does

let Node { id, tags, decimicro_lat, decimicro_lon } = node;

this code will not compile if the feature is activated.

We can keep the field and only populate it with an parameter, but the Info struct will make the different structs significantly bigger. We can use pub info: Option<Box<Info>>, to limit the augmented size, but it will impact the performance with and without the Info retrieval.

What do you propose?

[jocelynj]

I didn't know of the rule that enabling a feature shouldn't break logic, but it makes sense.

There is also the possibility of adding functions like iter_with_metadata, but it might mean to duplicate most of the logic. Or maybe we could play with generics, but might be too complex.

I will do some trials with your proposition of info: Option<Box<Info>>.

In any case, does it mean that there will be a need to bump the major version of the package? My original thought was that adding a new feature would help to not break backward compatibility.

[TeXitoi]

If we modify a struct with all pub field, yes, it will need a version increment.

Another possiblitity would be to encapsulate in the other way:

struct WithInfo<T> {
    obj: T,
    info: Option<Info>,
}

And iterate on it, with dedicated methods.

[jocelynj]

I have launched examples/count.rs on france.osm.pbf, with Option<Box<Info>>, and it is slower than expected. These stats are generated with hyperfine

• main
  Time (mean ± σ): 18.375 s ± 1.249 s [User: 129.637 s, System: 4.216 s]
  Range (min … max): 17.712 s … 21.905 s 10 runs

• this version, with full-metadata off
  Time (mean ± σ): 18.196 s ± 0.307 s [User: 130.923 s, System: 4.132 s]
  Range (min … max): 17.900 s … 18.967 s 10 runs

• this version, with full-metadata on
  Time (mean ± σ): 24.424 s ± 0.363 s [User: 181.625 s, System: 5.445 s]
  Range (min … max): 24.039 s … 25.108 s 10 runs

• this version, but using NonZero<> in Info, with full-metadata on
  Time (mean ± σ): 21.686 s ± 0.255 s [User: 152.626 s, System: 4.924 s]
  Range (min … max): 21.476 s … 22.326 s 10 runs

• Option<Box<Info>>, with full-metadata off
  Time (mean ± σ): 19.162 s ± 0.440 s [User: 132.996 s, System: 4.502 s]
  Range (min … max): 18.824 s … 20.170 s 10 runs

• Option<Box<Info>>, with full-metadata on
  Time (mean ± σ): 35.523 s ± 0.286 s [User: 160.999 s, System: 5.323 s]
  Range (min … max): 35.129 s … 35.998 s 10 runs

• Option<Box<Info>>, using NonZero<> in Info, with full-metadata on
  Time (mean ± σ): 36.360 s ± 0.513 s [User: 162.979 s, System: 4.954 s]
  Range (min … max): 35.727 s … 37.125 s 10 runs

What would you think of making the info field private, with a size depending on the feature full-metadata, and adding a function get_info() to get its value?

[jocelynj]

I’ve changed the code to add a NodeInfo/WayInfo/RelationInfo structure, containing a separate Node/Way/Relation+Info fields. The previous functions iter() and par_iter() still return normal objects, and I’ve added new functions iter_with_metadata() and par_iter_with_metadata() to return objects with an Info field. I had to duplicate some logic, but maybe it would be better to make all the logic generic on Node/Way/Relation, as I did in src/groups.rs. I’m not a rust specialist, so the code might need some improvements :)

I’ve also modified Info to use Option<NonZero<...>>.

On the new code, performance is very good:

• full-metadata off
  Time (mean ± σ): 17.952 s ± 0.102 s [User: 128.248 s, System: 4.136 s]
  Range (min … max): 17.814 s … 18.108 s 10 runs

• full-metadata on, using par_iter()
  Time (mean ± σ): 18.551 s ± 0.115 s [User: 141.156 s, System: 4.206 s]
  Range (min … max): 18.424 s … 18.700 s 10 runs

• full-metadata on, using par_iter_with_metadata()
  Time (mean ± σ): 21.294 s ± 0.063 s [User: 147.180 s, System: 4.691 s]
  Range (min … max): 21.211 s … 21.386 s 10 runs

I think it is still worth keeping the optional feature, but we could remove it - I suspect that the additional logic in DenseNodes to update the denseinfo structure is the culprit for the small decrease in performance.

[jocelynj]

I’ve rewritten the code with your suggestion of using WithInfo<T>, and this time the normal build without the feature shouldn’t see any change, as the code is using new functions iter_with_info() and par_iter_with_info(). I could even remove the feature full-metadata if you want me to.

I didn’t add any equivalent function to nodes/ways/relations iterators - is it necessary?

[TeXitoi]

Sorry to take time for reviewing, and thanks for your perseverence.

No need to add additional methods, they can always be added after.

[jocelynj]

I see that I have forgotten to switch the Info struct to NonZero for version/timestamp/changeset/uid. I think it is worth doing, as it gives ~9% more performance (22.6s instead of 24.9 for France pbf). Do you want me to add a commit on this branch to do the switch?

Something like this:

 pub struct Info {
     /// The version of the object.
-    pub version: Option<i32>,
+    pub version: Option<NonZero<i32>>,
     /// The timestamp when the object was last modified.
-    pub timestamp: Option<i64>,
+    pub timestamp: Option<NonZero<i64>>,
     /// The changeset id of the last modification.
-    pub changeset: Option<i64>,
+    pub changeset: Option<NonZero<i64>>,
     /// The user id of the last user who modified this object.
-    pub uid: Option<i32>,
+    pub uid: Option<NonZero<i32>>,

[TeXitoi]

Yes please as it is a breaking change.

[jocelynj]

I’ve pushed a commit to use NonZero.

There is another potential change that I’m really not sure about, which is to change WithInfo<T> to use directly Info instead of Option<Info>. This gives another 1s of perf improvement, but I’m wondering if it makes the logic a bit less clean. Do you want me to push this change too?

[jocelynj]

After thinking a bit more about it, using Info instead of Option makes more sense, as we now have dedicated functions to read a PBF with metadata. Please tell if you want me to remove the feature "full-metadata" or keep the code as it is now :)