Replace Luabind with Sol2 #3382

karenzshea · 2016-11-30T17:17:57Z

Issue

We're running on Luabind and it's a constant pain not only for us but also for users. As soon as we manage to port our code we will switch over to sol2 and never look back.

Tasklist

Requirements / Relations

This is a pre-MLD task.

daniel-j-h · 2016-11-30T17:28:25Z

src/extractor/scripting_environment_sol2.cpp

+
+    // TODO: do we want this?
+    //"get_nodes",
+    //&osmium::Way::nodes);


Yeah we need to have a 1:1 feature parity with our old Luabind code. Please go over both impls. and check this.

(Sorry that was me being lazy not registering all functions in the initial pass)

daniel-j-h · 2016-11-30T17:29:31Z

src/extractor/scripting_environment_sol2.cpp

+    context.state.new_type<RasterDatum>("RasterDatum",
+            "datum", &RasterDatum::datum,
+            "invalid_data", &RasterDatum::get_invalid);
+    */


Same here before we can merge this.

emiltin · 2016-11-30T20:03:23Z

src/extractor/scripting_environment_sol2.cpp

+{
+    auto get_value_by_key(const char *key, const char *default_value = nullptr) const
+    {
+        return way.get_value_by_key(key, default_value);


note the recent change to return null instead of empty tags

daniel-j-h · 2016-12-05T13:44:40Z

CMakeLists.txt

@@ -91,6 +91,7 @@ endif()

 include_directories(BEFORE ${CMAKE_CURRENT_BINARY_DIR}/include/)
 include_directories(BEFORE ${CMAKE_CURRENT_SOURCE_DIR}/include/)
+include_directories(SYSTEM ${CMAKE_CURRENT_SOURCE_DIR}/third_party/)


Hm can you vendor the package at third_party/sol2/ add this path here and then

third_party/sol2/sol2/sol.hpp ^ includedir points here

so the user can still do include <sol2/sol.hpp>

also below: can you rebase on master, all access restricted tags should be gone by now - if not please ping me

Already rebased, which is how I picked up on the missing sol directory includes. I'll update how the sol header is saved and update in the cmakelists now.

MoKob · 2016-12-05T14:18:32Z

include/extractor/extraction_way.hpp

@@ -46,6 +46,7 @@ struct ExtractionWay
        roundabout = false;
        circular = false;
        is_startpoint = true;
+        ignore_in_grid = false;


Could this be left-over code? I wanted to check for it but could not find it in the remaining parts of the PR.

I added this field. Sol was erroring in places where we set this property in bicycle/foot profiles but hadn't registered a corresponding field in the bindings. I'm not sure how luabind handled this before.

I did some digging. Seems the profiles are out of date. That flag was removed in faa880d

Capturing from chat: Next actions:

remove dead flag (ignore_in_grid) from foot/bicycle profile

remove bindings for (ignore_in_grid)

remove ignore_in_grid from extraction_way.hpp

karenzshea · 2016-12-06T21:08:44Z

I checked compatibility with Lua 5.1 and 5.2 by building osrm with both versions and running tests. They check out OK; if there's any further testing I should do, lmk @MoKob @daniel-j-h

karenzshea · 2016-12-06T21:31:54Z

@daniel-j-h @TheMarex review?

TheMarex

Almost read for merging once we pushed 5.5 out. 🎉 👋 luabind. We should also mark https://github.com/mapbox/luabind as deprecated since we won't support this anymore.

TheMarex · 2016-12-06T22:50:08Z

CMakeLists.txt

@@ -282,15 +282,15 @@ endif()

 # Configuring compilers
 if(${CMAKE_CXX_COMPILER_ID} STREQUAL "Clang")
-  set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -Wextra -pedantic -Wuninitialized -Wunreachable-code -Wstrict-overflow=2 -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fPIC -fcolor-diagnostics")
+  set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -Wextra -pedantic -Wuninitialized -Wunreachable-code -Wstrict-overflow=2 -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fPIC -fcolor-diagnostics -ftemplate-depth=1024")


clang has a default limit of 256, gcc has 1024 - this is fine 👍

TheMarex · 2016-12-06T22:52:01Z

include/extractor/scripting_environment_sol2.hpp

-#define SCRIPTING_ENVIRONMENT_LUA_HPP
-
-#include "extractor/scripting_environment.hpp"
+#ifndef SCRIPTING_ENVIRONMENT_SOL2_HPP


Hm why did we rename this file? Sure the library that we use for executing lua changed but this is still for executing lua code.

I did this back a month ago when I was experimenting with sol2 and still had the luabind files around - this can change now

TheMarex · 2016-12-06T22:55:53Z

src/extractor/scripting_environment_sol2.cpp

+    }
+}
+
+void Sol2ScriptingContext::processNode(const osmium::Node &node, ExtractionNode &result)


Naming convention would be ProcessNode.

TheMarex · 2016-12-06T22:56:04Z

src/extractor/scripting_environment_sol2.cpp

+    node_function(node, result);
+}
+
+void Sol2ScriptingContext::processWay(const osmium::Way &way, ExtractionWay &result)


Naming convention would be ProcessWay.

TheMarex · 2016-12-06T23:00:43Z

CMakeLists.txt

@@ -473,6 +465,33 @@ else()
    find_package(GDAL)
  endif()

+  FIND_PACKAGE(Lua 5.2 EXACT)


I think we can now simplify all this logic down to find_package(Lua 5.1) which will look for lua 5.1+. Sol seems to have a good support for all lua versions, unlike luabind.

The check here was because of the Lua 5.3 memory issue and not specifically for Luabind - until we confirm the Lua 5.3 memory issues are gone simply by using sol we should keep it.

TheMarex · 2016-12-06T23:01:38Z

include/extractor/extraction_way.hpp

@@ -96,6 +96,7 @@ struct ExtractionWay
    bool roundabout;
    bool circular;
    bool is_startpoint;
+    bool ignore_in_grid;


This seems like an accidental change.

yes this has to go - not only here but also in the profiles and the bindings, see #3382 (review)

daniel-j-h · 2016-12-13T15:00:53Z

Windows build are failing:

C:\projects\osrm\third_party\sol2\sol2/sol.hpp(771): fatal error C1083: Cannot open include file: 'lua.hpp': No such file or directory [C:\projects\osrm\build\EXTRACTOR.vcxproj]

Seems like the C++ lua.hpp wrapper can't be found on Windows. cc @BergWerkGIS

karenzshea · 2016-12-13T15:55:27Z

Timing the switch:

ran extraction the germany geofabrik extract
r3.8xlarge machine, 32 cores

branch	extraction time	parsing time
bye-luabind	real 15m41.392s, user 39m24.131s, sys 16m7.835s	130.264
master	real 16m25.099s, user 58m34.169s, sys 28m56.980s	196.322s

daniel-j-h · 2016-12-13T16:00:01Z

These are great numbers and will lift some pressure from the guidance code!

(Unrelated to this PR: the next step would be to instead of context switching for every node or way, we switch on a nodes-chunk and ways-chunk basis => #1777)

wilhelmberg · 2016-12-13T16:30:53Z

@daniel-j-h where is lua.hpp coming from on other platforms?
On Windows we clone https://github.com/LuaDist/lua
The only one I could find in this repo is https://github.com/LuaDist/lua/blob/lua-5.1/etc/lua.hpp
But we don't include it - would it be the one we need?

daniel-j-h · 2016-12-13T16:32:35Z

Yes this looks like the file we need. It's just setting up the includes for the C name mangling.

wilhelmberg · 2016-12-14T08:52:43Z

@daniel-j-h could you try again - I've updated and rebuilt that binary deps package.

lua.hpp is now in osrm-deps\libs\include:

…rs in the tag.

daniel-j-h · 2016-12-14T09:19:09Z

Rebased against master and force-pushed. Let's see if Appveyor is happy with it now. cc @karenzshea

daniel-j-h · 2016-12-14T13:17:16Z

@BergWerkGIS thanks, this resolved it. Ready for merge now! 🎉

daniel-j-h · 2016-12-15T10:24:49Z

It's gone, Luabind is gone - I can't believe it! :)

Updating the Wiki pages now.

daniel-j-h reviewed Nov 30, 2016

View reviewed changes

karenzshea force-pushed the bye-luabind branch from 6391a29 to 958354a Compare November 30, 2016 17:41

karenzshea added the Work In Progress label Nov 30, 2016

emiltin reviewed Nov 30, 2016

View reviewed changes

emiltin mentioned this pull request Nov 30, 2016

optimize tag access from LUA profiles #3369

Closed

daniel-j-h added this to the 5.6.0 milestone Dec 5, 2016

karenzshea force-pushed the bye-luabind branch from 479a2bb to c67cb66 Compare December 5, 2016 13:27

daniel-j-h reviewed Dec 5, 2016

View reviewed changes

MoKob reviewed Dec 5, 2016

View reviewed changes

karenzshea force-pushed the bye-luabind branch 4 times, most recently from e715643 to 3c7834f Compare December 6, 2016 15:58

karenzshea added the Pre-MLD label Dec 6, 2016

karenzshea force-pushed the bye-luabind branch from 745d37c to 6ca1521 Compare December 6, 2016 17:23

karenzshea added the Review label Dec 6, 2016

karenzshea force-pushed the bye-luabind branch from 6ca1521 to 769a6ab Compare December 6, 2016 21:29

TheMarex removed the Work In Progress label Dec 6, 2016

TheMarex requested changes Dec 6, 2016

View reviewed changes

karenzshea force-pushed the bye-luabind branch 2 times, most recently from ed88ad5 to 945f1ce Compare December 7, 2016 14:13

TheMarex approved these changes Dec 7, 2016

View reviewed changes

karenzshea added Ready To Merge and removed Review labels Dec 8, 2016

karenzshea force-pushed the bye-luabind branch from 1909893 to 748895b Compare December 12, 2016 16:07

daniel-j-h added the Work In Progress label Dec 13, 2016

daniel-j-h removed the Ready To Merge label Dec 13, 2016

daniel-j-h mentioned this pull request Dec 14, 2016

Report LUA errors #3084

Open

danpat and others added 5 commits December 14, 2016 10:02

Don't mark all lanes as designated when there are blank lane specifie…

776356c

…rs in the tag.

Clean plate: luabind ripped out and sol2 basic integration

80a46ae

Rewrite lua bindings using sol interface

ea92e9d

increase template depth on clang and gcc builds

bf7cfb3

switch back to lua based env naming

fb81ec2

daniel-j-h force-pushed the bye-luabind branch from 748895b to fb81ec2 Compare December 14, 2016 09:18

daniel-j-h added Ready To Merge and removed Work In Progress labels Dec 14, 2016

TheMarex merged commit 68e3888 into master Dec 15, 2016

TheMarex deleted the bye-luabind branch December 15, 2016 09:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace Luabind with Sol2 #3382

Replace Luabind with Sol2 #3382

karenzshea commented Nov 30, 2016 •

edited by daniel-j-h

Loading

daniel-j-h Nov 30, 2016

daniel-j-h Nov 30, 2016

emiltin Nov 30, 2016

daniel-j-h Dec 5, 2016

daniel-j-h Dec 5, 2016

karenzshea Dec 5, 2016

MoKob Dec 5, 2016

karenzshea Dec 5, 2016

MoKob Dec 5, 2016

MoKob Dec 5, 2016 •

edited

Loading

karenzshea commented Dec 6, 2016

karenzshea commented Dec 6, 2016

TheMarex left a comment

TheMarex Dec 6, 2016

daniel-j-h Dec 7, 2016

TheMarex Dec 6, 2016

daniel-j-h Dec 7, 2016

TheMarex Dec 6, 2016

TheMarex Dec 6, 2016

TheMarex Dec 6, 2016

daniel-j-h Dec 7, 2016

TheMarex Dec 6, 2016

daniel-j-h Dec 7, 2016

daniel-j-h commented Dec 13, 2016

karenzshea commented Dec 13, 2016

daniel-j-h commented Dec 13, 2016

wilhelmberg commented Dec 13, 2016

daniel-j-h commented Dec 13, 2016

wilhelmberg commented Dec 14, 2016

daniel-j-h commented Dec 14, 2016

daniel-j-h commented Dec 14, 2016

daniel-j-h commented Dec 15, 2016

Replace Luabind with Sol2 #3382

Replace Luabind with Sol2 #3382

Conversation

karenzshea commented Nov 30, 2016 • edited by daniel-j-h Loading

Issue

Tasklist

Requirements / Relations

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

MoKob Dec 5, 2016 • edited Loading

Choose a reason for hiding this comment

karenzshea commented Dec 6, 2016

karenzshea commented Dec 6, 2016

TheMarex left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniel-j-h commented Dec 13, 2016

karenzshea commented Dec 13, 2016

daniel-j-h commented Dec 13, 2016

wilhelmberg commented Dec 13, 2016

daniel-j-h commented Dec 13, 2016

wilhelmberg commented Dec 14, 2016

daniel-j-h commented Dec 14, 2016

daniel-j-h commented Dec 14, 2016

daniel-j-h commented Dec 15, 2016

karenzshea commented Nov 30, 2016 •

edited by daniel-j-h

Loading

MoKob Dec 5, 2016 •

edited

Loading