Unify fingerprinting & Count64 #3904

duizendnegen · 2017-04-05T13:45:22Z

Issue

#3520

Tasklist

convert serialization to use fingerprint generation and validation
replace Count32 with Count64
review
adjust for comments

Requirements / Relations

This should go in after #3892 (the other way around is too much hassle)

duizendnegen · 2017-04-07T10:04:28Z

src/storage/storage.cpp

-        io::FileReader reader(config.geometries_path, io::FileReader::HasNoFingerprint);
+        io::FileReader reader(config.geometries_path, io::FileReader::VerifyFingerprint);

        const auto number_of_geometries_indices = reader.ReadVectorSize<unsigned>();


What is the logic behind ReadVectorSize reading CountElement64, then skipping T (in this case unsigned)? Naming doesn't cover what's actually happening here.

Ideally I'd also take this through a files layer, any input as to how?

We need to extract the file size in order to pre-allocate the shared memory block. For this we need to extract the size of every block/vector.

I couldn't come up with a good abstraction for this yet. But yeah having a dedicated function in files:: would be good.

duizendnegen · 2017-04-07T16:45:09Z

include/engine/engine.hpp

        auto mem = storage::makeSharedMemory(barrier.data().region);
        auto layout = reinterpret_cast<storage::DataLayout *>(mem->Ptr());
-        return layout->GetBlockSize(storage::DataLayout::CH_CORE_MARKER) > 4;
+        return layout->GetBlockSize(storage::DataLayout::CH_CORE_MARKER) > 16;


This (and underlying sizeof(std::uint64_t) + sizeof(util::FingerPrint);) can really bite... Any suggestions for a more understandable naming, to get rid of these magic numbers?

The least we should do is add some comments to both sections.

duizendnegen · 2017-04-07T16:47:18Z

Would like to unify file names between storage_config and extract_config - any objections?

danpat · 2017-04-07T21:00:25Z

@duizendnegen no objections - we have way too much repetition

… function Fingerprint-aware

duizendnegen · 2017-04-08T15:31:44Z

src/updater/updater.cpp

    using boost::interprocess::mapped_region;

+    {
+        storage::io::FileReader file(filename, storage::io::FileReader::VerifyFingerprint);


Opening this file with the FileReader just for verifying the fingerprint seems a bit clumsy. On Windows this fails in a way since a subsequent call from file_mapping results in Access denied.

Is there a way to read just the Fingerprint from the mapped_region? Should we implement an explicit Close function for the FileReader/FileWriter, or do an implicit fclose on the destructor?

or do an implicit fclose on the destructor

Wait we don't close file handles? This is definitely a bug that should be fixed.

/cc @danpat I think these might be the reason we saw the file handle exhaustion.

Okay just looked at the implementation: We just wrap ifstream which closes on destruction, which still begs the question of why this fails here.

Yes, it should be close-on-destroy. The errors on appveyor seem to be around mmap calls though - perhaps we're not unmapping at some point? Multiple mappings should work though.....

I was wrong anyway, commented out the file reader fingerprint verification and errors still occur. I'll skip fingerprinting these mmap'ed files for now (just one file) to get this to land and open a ticket.

duizendnegen · 2017-04-08T19:29:30Z

Ready for review / feedback. I'm tackling config normalization in a separate PR.

TheMarex

Looks good to me, but travis is failing because the formatting is broken.

TheMarex · 2017-04-12T20:33:37Z

src/contractor/contractor.cpp

-    const auto level_size = order_file.ReadElementCount32();
-    node_levels.resize(level_size);
-    order_file.ReadInto(node_levels);
+    files::readLevels(config.level_output_path, node_levels);


Thanks for the refactor! I think we can remove Contractor::ReadNodeLevels entirely now and directly call files::readLevels.

TheMarex · 2017-04-12T20:33:47Z

src/contractor/contractor.cpp

-                                   storage::io::FileWriter::HasNoFingerprint);
-
-    storage::serialization::write(writer, node_levels);
+    files::writeLevels(config.level_output_path, node_levels);


Same as above.

TheMarex · 2017-04-12T20:47:19Z

src/storage/storage.cpp

-        io::FileReader reader(config.geometries_path, io::FileReader::HasNoFingerprint);
+        io::FileReader reader(config.geometries_path, io::FileReader::VerifyFingerprint);

        const auto number_of_geometries_indices = reader.ReadVectorSize<unsigned>();


We need to extract the file size in order to pre-allocate the shared memory block. For this we need to extract the size of every block/vector.

I couldn't come up with a good abstraction for this yet. But yeah having a dedicated function in files:: would be good.

TheMarex · 2017-04-12T20:48:32Z

src/updater/updater.cpp

-        mapped_region region{mapping, mode};
+
+        // map region started at an offset of util::FingerPrint size
+        mapped_region region{mapping, mode, 8};


instead of 8 what about using sizeof(Fingerprint)?

TheMarex · 2017-04-12T20:51:51Z

Windows tests seem to fail during updating. I suspect the 8 bytes for the fingerprint are not working cross platform?

duizendnegen · 2017-04-13T09:02:04Z

@TheMarex it's not about the 8 bytes part - see #3904 (comment) - it's opening and not explicitly closing the file, then re-opening (somehow). To get things to work I can leave out fingerprint checking .turn_penalties_index, or figure out a different way to read the fingerprint, or close the file explicitly somewhere. Open for suggestions.

…teration because of Appveyor issues.

duizendnegen · 2017-04-18T10:51:03Z

Skipped the problematic .turn_penalties_index for another PR. Ready to merge from my side.

Fingerprint and move .levels

196742c

duizendnegen force-pushed the feature/fingerprint-all-files branch from bc8647d to 196742c Compare April 7, 2017 09:02

duizendnegen added 2 commits April 7, 2017 11:25

Fingerprint .osrm.datasource_names

3fb2bf3

Fingerprint geometries

de215ef

duizendnegen commented Apr 7, 2017

View reviewed changes

duizendnegen added 3 commits April 7, 2017 13:34

Fingerprint .edges

d9abcb0

fingerprint .tld

c98e0a4

fingerprint .ramIndex / tree nodes

da4387e

duizendnegen mentioned this pull request Apr 7, 2017

Count32 to Count64 #3848

Closed

3 tasks

Fingerprint .core

77427de

duizendnegen commented Apr 7, 2017

View reviewed changes

duizendnegen changed the title ~~Feature/fingerprint all files~~ Unify fingerprinting, Count64, storage_config Apr 8, 2017

duizendnegen added 7 commits April 8, 2017 13:37

Fingerprint .timestamp, normalize use of Size / GetSize and make that…

9906790

… function Fingerprint-aware

Fingerprint .names

b656664

Fingerprint .turn_duration_penalties and .turn_weight_penalties

49edebd

Fingerprint .tld

34b0d56

Fingerprint .properties

12148f9

Fingerprint .turn_penalties_index

3e705e5

Replace Count32 with Count64

e445ddc

duizendnegen commented Apr 8, 2017

View reviewed changes

duizendnegen force-pushed the feature/fingerprint-all-files branch from dd08505 to e445ddc Compare April 8, 2017 19:28

duizendnegen mentioned this pull request Apr 8, 2017

Normalize config files #3920

Merged

9 tasks

duizendnegen changed the title ~~Unify fingerprinting, Count64, storage_config~~ Unify fingerprinting & Count64 Apr 12, 2017

TheMarex approved these changes Apr 12, 2017

View reviewed changes

TheMarex added the Review - In feedback label Apr 12, 2017

duizendnegen added 3 commits April 13, 2017 11:45

Address PR comments

a165d29

clang-format

b3977fe

Skip fingerprinting .turn_penalties_index, to be done in a separate i…

c041afa

…teration because of Appveyor issues.

duizendnegen force-pushed the feature/fingerprint-all-files branch from 57afed6 to c041afa Compare April 18, 2017 08:18

duizendnegen mentioned this pull request Apr 18, 2017

Put fingerprint on .turn_penalties_index file #3951

Closed

TheMarex merged commit e85c4f8 into Project-OSRM:master Apr 18, 2017

Unify fingerprinting & Count64 #3904

Unify fingerprinting & Count64 #3904

Uh oh!

Conversation

duizendnegen commented Apr 5, 2017 • edited by TheMarex Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue

Tasklist

Requirements / Relations

Uh oh!

duizendnegen Apr 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

duizendnegen commented Apr 7, 2017

Uh oh!

danpat commented Apr 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheMarex Apr 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

duizendnegen commented Apr 8, 2017

Uh oh!

TheMarex left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheMarex commented Apr 12, 2017

Uh oh!

duizendnegen commented Apr 13, 2017

Uh oh!

duizendnegen commented Apr 18, 2017

Uh oh!

Uh oh!

duizendnegen commented Apr 5, 2017 •

edited by TheMarex

Loading

duizendnegen Apr 7, 2017 •

edited

Loading

TheMarex Apr 13, 2017 •

edited

Loading