Landmark Routing 1 PR: make landmark db operations a pimpl #4202

kevinkreiser · 2023-07-14T13:25:45Z

The pimpl idiom is a powerful one especially for api design. It allows the designer to offer up the api interface without exposing the details of how its implemented, effectively giving the api a PrivateIMPLementation.

I think this best suits pretty much all of our c++ apis but sadly not all of them adhere to this. At any rate this is an attempt to refactor the recent work in #4189 to model this.

In addition to the pimpl I cut down logging as much as possible, I only close the db in the destructor and instead of returning booleans I throw when there is a problem that is unrecoverable

By and large the code is pretty much the same just leaner and meaner

kevinkreiser · 2023-07-14T13:30:46Z

CHANGELOG.md

@@ -15,6 +15,7 @@
   * CHANGED: write traffic tile headers in `valhalla_build_extract` [#4195](https://github.com/valhalla/valhalla/pull/4195)
   * ADDED: `source_percent_along` & `target_percent_along` to /trace_attributes JSON response [#4199](https://github.com/valhalla/valhalla/pull/4199)
   * ADDED: sqlite database to store landmarks along with interfaces of insert and bounding box queries [#4189](https://github.com/valhalla/valhalla/pull/4189)
+   * CHANGED: refactor landmark database interface to use a pimpl [#4202](https://github.com/valhalla/valhalla/pull/4202)


@vesperlou the diff for the source files below is large so its hard to read. i would suggest just looking at the files directly (in the 3 dots on the side you can click "view file")

ok, will take a look

kevinkreiser · 2023-07-14T13:32:37Z

valhalla/mjolnir/landmark_database_builder.h

+  LandmarkDatabase(const std::string& db_name, bool read_only);
+  void insert_landmark(const Landmark& landmark);
  std::vector<Landmark> get_landmarks_in_bounding_box(const double minLat,


the interface becomes just the functions that we want to publicly expose. the only thing the outside caller can see that is protected is a pimpl which gives no clue about its implementation

got it. I have studied C++ PImpl. nice job, thanks!

kevinkreiser · 2023-07-14T13:33:50Z

src/mjolnir/landmark_database_builder.cc

+// TODO: this can be a utility and be more generic with a few more options, we could make the prepared
+//  statements on the fly and retrievable by the caller, then anything in the code base that wants to
+//  use sqlite can make use of this utility class. for now its ok to be specific to landmarks though
+struct LandmarkDatabase::db_pimpl {


i think this class could get more generic and let users of it register prepared statements, retrieve them and run them. then we could use this class for any sqlite needs. for now i didnt go that far but if we do then this can go to a private header

kevinkreiser · 2023-07-14T13:37:09Z

src/mjolnir/landmark_database_builder.cc

+  std::shared_ptr<void> spatial_lite;
+  bool vacuum_analyze = false;
+
+  db_pimpl(const std::string& db_name, bool read_only) : insert_stmt(nullptr) {


the constructor does all the initialization stuff directly rather than having myriad functions. if anything goes wrong it throws. if something upstream wants to catch what is thrown and log it fine. i would expect the graph enhancer for example to catch, log that i couldnt open the landmarks database and that its skipping adding landmarks to the tiles. for the writer part i would just let the throw abort the process (ie dont catch)

make sense.

kevinkreiser · 2023-07-14T13:39:02Z

src/mjolnir/landmark_database_builder.cc

@@ -164,7 +148,7 @@ std::vector<Landmark> LandmarkDatabase::get_landmarks_in_bounding_box(const doub
  sqlite3_bind_double(bounding_box_stmt, 3, maxLong);
  sqlite3_bind_double(bounding_box_stmt, 4, maxLat);

-  LOG_INFO(sqlite3_expanded_sql(bounding_box_stmt));
+  LOG_TRACE(sqlite3_expanded_sql(bounding_box_stmt));


i pretty much removed all logs except:

the destructor logs when vacuum analyze fails

insert/select log trace now because anything else is too verbose

I feel the same.

kevinkreiser · 2023-07-15T12:51:24Z

@vesperlou @nilsnolde do either of you have time to look this pr over?

vesperlou · 2023-07-14T15:37:44Z

valhalla/mjolnir/landmark_database_builder.h

+  LandmarkDatabase(const std::string& db_name, bool read_only);
+  void insert_landmark(const Landmark& landmark);
  std::vector<Landmark> get_landmarks_in_bounding_box(const double minLat,


got it. I have studied C++ PImpl. nice job, thanks!

vesperlou · 2023-07-14T15:43:45Z

valhalla/mjolnir/landmark_database_builder.h

+#include <memory>
+#include <string>
+#include <vector>


only include the necessary, basic standard libraries to avoid unused imports?

if you look below these are the only things the interface makes use of, IWYU

vesperlou · 2023-07-14T15:49:58Z

src/mjolnir/landmark_database_builder.cc

+  std::shared_ptr<void> spatial_lite;
+  bool vacuum_analyze = false;
+
+  db_pimpl(const std::string& db_name, bool read_only) : insert_stmt(nullptr) {


make sense.

vesperlou · 2023-07-14T15:55:21Z

src/mjolnir/landmark_database_builder.cc

@@ -164,7 +148,7 @@ std::vector<Landmark> LandmarkDatabase::get_landmarks_in_bounding_box(const doub
  sqlite3_bind_double(bounding_box_stmt, 3, maxLong);
  sqlite3_bind_double(bounding_box_stmt, 4, maxLat);

-  LOG_INFO(sqlite3_expanded_sql(bounding_box_stmt));
+  LOG_TRACE(sqlite3_expanded_sql(bounding_box_stmt));


I feel the same.

vesperlou · 2023-07-14T15:56:30Z

src/mjolnir/landmark_database_builder.cc

+    throw std::runtime_error("Sqlite could not query landmarks in bounding box: " +
+                             pimpl->last_error());


before throw an exception we don't need to close database connection?

nope, the connection will be closed when the object is destructed no need to close it ahead of time.

but we need to finalize statement before destructing the object?

yep the finalize statement is in the destructor above before the call to close the connection

vesperlou · 2023-07-15T13:33:15Z

@vesperlou @nilsnolde do either of you have time to look this pr over?

@kevinkreiser I reviewed yesterday but didn't know I should submit it ooops ;) left 2 question marks above.

kevinkreiser · 2023-07-15T14:31:06Z

but didn't know I should submit it ooops

yeah that happens all the time, its a stupid UI design on githubs part

kevinkreiser · 2023-07-15T14:33:45Z

@vesperlou to finalize the review you can either select "approve" or "request changes" on the diff tab in the upper right hand side.

nilsnolde

LGTM thanks

vesperlou · 2023-07-15T14:43:09Z

@vesperlou to finalize the review you can either select "approve" or "request changes" on the diff tab in the upper right hand side.

thanks! approved :)

kevinkreiser · 2023-07-15T18:48:03Z

src/mjolnir/landmark_database_builder.cc

-    sqlite3_free(err_msg);
-    sqlite3_close(db);
-    return false;
+    sqlite3_finalize(insert_stmt);


here you can see we finalize the statements and then close the connection

sure. I am actually wondering why we should explicitly finalize statements in destructor, because I thought they are going to be cleaned automatically when the object is destroyed, like before throwing we dont have to close the connection.

like the db connection the prepared statements are pointing to some memory that was dynamically allocated. in order for us to not "leak" memory we do need to manually deallocate that memory. when we throw the call immediately bubbles that exception upwards however the rules about what happens to objects when their life time ends still apply. that means if the outside scope that caused the exception was the scope that owned the LandmarkDatabase object lets that object go out of scope, its destructor (here) will be called and make sure to clean up the connection and the prepared statements.

if you mean closing the connection should finalize any statements, i dont think so. prepared statemnts are not tied to any connection they are a wholly separate construct. from the manual:

The application must finalize every prepared statement in order to avoid resource leaks.

oh I see. I mixed up some concepts earlier but now I understand everything.
thanks for the detailed explanation!

kevinkreiser added 4 commits July 14, 2023 09:09

simplify original class and prepare for pimpl

4686d92

finish pimpl

06f1440

formant

0cd61fc

changelog

6095752

kevinkreiser requested a review from nilsnolde July 14, 2023 13:27

kevinkreiser commented Jul 14, 2023

View reviewed changes

kevinkreiser requested a review from vesperlou July 14, 2023 13:48

vesperlou reviewed Jul 15, 2023

View reviewed changes

nilsnolde approved these changes Jul 15, 2023

View reviewed changes

vesperlou approved these changes Jul 15, 2023

View reviewed changes

kevinkreiser commented Jul 15, 2023

View reviewed changes

kevinkreiser merged commit d544de3 into master Jul 15, 2023

kevinkreiser deleted the kk_simplify_landmarks branch July 15, 2023 18:48

vesperlou changed the title ~~make landmark db operations a pimpl~~ Landmark Routing 1 PR: make landmark db operations a pimpl Sep 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Landmark Routing 1 PR: make landmark db operations a pimpl #4202

Landmark Routing 1 PR: make landmark db operations a pimpl #4202

kevinkreiser commented Jul 14, 2023 •

edited

Loading

kevinkreiser Jul 14, 2023

vesperlou Jul 14, 2023

kevinkreiser Jul 14, 2023

vesperlou Jul 14, 2023

kevinkreiser Jul 14, 2023

kevinkreiser Jul 14, 2023

vesperlou Jul 14, 2023

kevinkreiser Jul 14, 2023

vesperlou Jul 14, 2023

kevinkreiser commented Jul 15, 2023

vesperlou Jul 14, 2023

vesperlou Jul 14, 2023

kevinkreiser Jul 15, 2023

vesperlou Jul 15, 2023

vesperlou Jul 14, 2023

vesperlou Jul 14, 2023

vesperlou Jul 14, 2023

kevinkreiser Jul 15, 2023

vesperlou Jul 15, 2023 •

edited

Loading

kevinkreiser Jul 15, 2023

vesperlou commented Jul 15, 2023

kevinkreiser commented Jul 15, 2023

kevinkreiser commented Jul 15, 2023 •

edited

Loading

nilsnolde left a comment

vesperlou commented Jul 15, 2023

kevinkreiser Jul 15, 2023

vesperlou Jul 17, 2023

kevinkreiser Jul 17, 2023

vesperlou Jul 17, 2023

		throw std::runtime_error("Sqlite could not query landmarks in bounding box: " +
		pimpl->last_error());

Landmark Routing 1 PR: make landmark db operations a pimpl #4202

Landmark Routing 1 PR: make landmark db operations a pimpl #4202

Conversation

kevinkreiser commented Jul 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinkreiser commented Jul 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vesperlou Jul 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vesperlou commented Jul 15, 2023

kevinkreiser commented Jul 15, 2023

kevinkreiser commented Jul 15, 2023 • edited Loading

nilsnolde left a comment

Choose a reason for hiding this comment

vesperlou commented Jul 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinkreiser commented Jul 14, 2023 •

edited

Loading

vesperlou Jul 15, 2023 •

edited

Loading

kevinkreiser commented Jul 15, 2023 •

edited

Loading