Vector tile 2.x, sort during by mapsam · Pull Request #42 · mapbox/vtquery

mapsam · 2017-11-22T17:52:53Z

Integrating vector tile 2.x and a "sort during"

What changed?

using a std::priority_queue for the ResultObject references, which is sorted upon emplacing into the queue, rather than sorting after the entire loop finishes
integrate vector-tile 2.x branch to decode geometry and properties for us (removes the geometry_processors.cpp file) - includes a new visitor function for decoding variants into v8 values
adds a separate code path for radius 0.0, resolves radius=0 leads to zero results with PIP #36 (closing Check for "within" results from closest_point #41)

benchmarks

this branch

UV_THREADPOOL_SIZE=1 node bench/vtquery.bench.js --iterations=1000 --concurrency=1

1: pip: many building polygons ... 956 runs/s (1046ms)
2: pip: many building polygons, single layer ... 1053 runs/s (950ms)
3: query: many building polygons, single layer ... 928 runs/s (1078ms)
4: query: linestrings, mapbox streets roads ... 1709 runs/s (585ms)
5: query: polygons, mapbox streets buildings ... 935 runs/s (1069ms)
6: query: all things - dense single tile ... 510 runs/s (1962ms)
7: query: all things - dense nine tiles ... 65 runs/s (15418ms)
8: elevation: terrain tile nepal ... 849 runs/s (1178ms)
9: geometry: 2000 points in a single tile, no properties ... 2053 runs/s (487ms)
10: geometry: 2000 points in a single tile, with properties ... 1645 runs/s (608ms)
11: geometry: 2000 linestrings in a single tile, no properties ... 1086 runs/s (921ms)
12: geometry: 2000 linestrings in a single tile, with properties ... 952 runs/s (1050ms)
13: geometry: 2000 polygons in a single tile, no properties ... 690 runs/s (1450ms)
14: geometry: 2000 polygons in a single tile, with properties ... 643 runs/s (1556ms)

master

UV_THREADPOOL_SIZE=1 node bench/vtquery.bench.js --iterations=1000 --concurrency=1

1: pip: many building polygons ... 1100 runs/s (909ms)
2: pip: many building polygons, single layer ... 1248 runs/s (801ms)
3: query: many building polygons, single layer ... 356 runs/s (2806ms)
4: query: linestrings, mapbox streets roads ... 685 runs/s (1459ms)
5: query: polygons, mapbox streets buildings ... 362 runs/s (2766ms)
6: query: all things - dense single tile ... 242 runs/s (4127ms)
7: query: all things - dense nine tiles ... 39 runs/s (25482ms)
8: elevation: terrain tile nepal ... 947 runs/s (1056ms)
9: geometry: 2000 points in a single tile, no properties ... 1410 runs/s (709ms)
10: geometry: 2000 points in a single tile, with properties ... 511 runs/s (1956ms)
11: geometry: 2000 linestrings in a single tile, no properties ... 875 runs/s (1143ms)
12: geometry: 2000 linestrings in a single tile, with properties ... 423 runs/s (2366ms)
13: geometry: 2000 polygons in a single tile, no properties ... 715 runs/s (1398ms)
14: geometry: 2000 polygons in a single tile, with properties ... 378 runs/s (2646ms)

cc @flippmoke

mapsam · 2017-11-22T17:56:23Z

src/vtquery.cpp

-            v8::Local<v8::Object> tilequery_properties_obj = Nan::New<v8::Object>();
            for (auto const& prop : feature->properties) {
-                properties_obj->Set(Nan::New<v8::String>(prop.first).ToLocalChecked(), get_property_value(prop.second));
+                set_property(prop, properties_obj);


@flippmoke this loop never ... loops. Am I referencing feature->properties correctly?

mapsam · 2017-11-22T17:57:14Z

src/vtquery.cpp

+            v8::Local<v8::Object> tilequery_properties_obj = Nan::New<v8::Object>();
            tilequery_properties_obj->Set(Nan::New("distance").ToLocalChecked(), Nan::New<v8::Number>(feature->distance));
            std::string og_geom = getGeomTypeString(feature->original_geometry_type);
            tilequery_properties_obj->Set(Nan::New("geometry").ToLocalChecked(), Nan::New<v8::String>(og_geom).ToLocalChecked());


@flippmoke The original geometry type string shows up in the final GeoJSON, but layer_name doesn't (one line lower). Not sure why.

This has been fixed

mapsam · 2017-11-22T18:39:55Z

This is currently blocked by mapbox/spatial-algorithms#21

mapsam · 2017-11-27T18:58:32Z

Spatial algorithms ☝️ branch has been merged and has a dev release on Mason. Onward.

mapsam · 2017-11-27T19:30:19Z

Looks like -weffc++ errors are throwing:

../src/vtquery.cpp:266:49: error: comparing floating point with == or != is unsafe [-Werror=float-equal]
                         if (cp_info.distance == 0.0) {
                                                 ^~~

but it looks like this isn't a big deal according to SO. I imagine I can bypass this by reversing the conditional to something like if (cp_info.distance >= 0.0) { ... }. Is this what you would do @flippmoke?

springmeyer · 2017-11-28T02:32:48Z

@mapsam - awesome work on this. Did a quick review and here are my thoughts:

do you have test coverage of sorted results? This PR looks like it changed the method of sorting and looks good to me - but do you have any tests that assert that the sorting of features is the same before and after? In particular do you have any tests that cover what happens when two or more features are the same exact distance? In this case what order do they end up in? Is it stable?
I noticed you can remove some repeated code in the geometry handling. How about applying this? https://gist.github.com/springmeyer/c3f88c31dc58e536f8b5dd5b8b015aaa
I noticed (unrelated to this PR) you have a string called uknown - is that misspelling intended?
The Nan::New<v8::Array> constructor takes a size, which will optimize its creation when you know the final size. In your case I think you do: you can pass Nan::New<v8::Array>(results_queue_.size()); in one case (when creating the features_array) and Nan::New<v8::Array>(2) in another case (when creating coordinates). This will not likely improve performance in any measurable way but will nevertheless be more efficient, so it is worth applying I think.

springmeyer

Made minor comments above. This looks great otherwise, so approving.

mapsam · 2017-11-28T19:35:01Z

do you have test coverage of sorted results?

I don't - that feels like a really good test to add. I'll work on updating one of them to check if the values are the same in the resulting feature collection.

I noticed you can remove some repeated code in the geometry handling. How about applying this? /springmeyer/c3f88c31dc58e536f8b5dd5b8b015aaa

Mind updating this link @springmeyer?

string called uknown - is that misspelling intended?

Indeed it is. I will update it 👌

My todos:

Nan::New<v8::Array> constructor size
update uknown spelling
implement @springmeyer's suggestion once I see it!
test: cover what happens when two or more features are the same exact distance
test: cover when results include a 0.0 point in polygon (ensure it is the first result)

springmeyer · 2017-11-28T23:27:16Z

@mapsam - okay, if the difference is in spatial-algos, then I'm comfortable with you applying a fuzzy comparison in vtquery rather than exact. It looks like this is what spatial-algos is doing by using BOOST_CHECK_CLOSE with an epsilon: https://github.com/mapbox/spatial-algorithms/blob/master/test/unit/closest_point/test-closest-point.cpp

mapsam · 2017-11-29T00:01:03Z

@springmeyer how does this look as a viable checkClose function?

function checkClose(a, b, epsilon) {
  return 1*(a-b) < epsilon;
}

And usage in tests with same epsilon as spatial-algorithms

assert.ok(checkClose(number_a, number_b, 1e-6));

springmeyer · 2017-11-29T00:21:44Z

looks great @mapsam!

…

On Nov 28, 2017, at 4:01 PM, Sam Matthews ***@***.***> wrote: @springmeyer <https://github.com/springmeyer> how does this look as a viable checkClose function? function checkClose(a, b, epsilon) { return 1*(a-b) < epsilon; } And usage in tests with same epsilon as spatial-algorithms assert.ok(checkClose(number_a, number_b, 1e-6)); — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#42 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABPTI_NbEgu79yEXLLMWK76cIdCH0cOks5s7J7BgaJpZM4Qnyhi>.

mapsam · 2017-11-29T00:26:23Z

Great! Okay, only one more thing I'm seeing - linux seems to return results with the same distance in a different order, despite the distance being the same number. Must they be in the same order? I would expect them to be in parsing order based on the operations of the priority queue, but this is hitting my expertise in how the priority queue resorts itself.

springmeyer · 2017-11-29T01:17:47Z

@mapsam - I'm not sure why, but I have a hunch and if I'm right @flippmoke can explain the details tomorrow. My hunch is that the sort is not "stable". In the past when using std::sort we've found we needed to move to http://en.cppreference.com/w/cpp/algorithm/stable_sort to get tests passing on osx and linux for fixtures. Since you are using priority_queue I wonder if https://lemire.me/blog/2017/03/13/stable-priority-queues/ is relevant. Again @flippmoke will know better than me, but this is a starting hunch.

mapsam · 2017-11-29T01:23:20Z

@springmeyer I lied so badly! 🙈

We're never using distance from closest_point as a return, and are only using it in the conditional comparison. What we're returning is a value from cheap-ruler based on latitude and longitude inputs. So basically we convert vector tile coordinates into latitude and longitude coordinates (cast to doubles), then use cheap-ruler to calculate the distance between them in meters with this function.

Numbers, as they move through vtquery (logged with std::clog):

# x,y
tilecoords:  601, 258
lnglat:      -87.7963, 41.8675
meters:      9.43636
ResObjDistance: 9.43636
js distance: 9.436356889343624

So, logging these numbers, it seems that closest_point and cheap-ruler work as expected and keep our numbers nice and rounded. It's when we pass from the v8 thread into a javascript value when we start getting sloppy. I think all javascript floating numbers are 64 bit, so not sure if there's anything we can do here other than continue asserting on the checkClose function?

mapsam · 2017-11-29T01:50:19Z

Just a confirmation that these tests are passing on macosx and not linux. Appears we cannot rely on order - @flippmoke have you seen priority_queue's react differently on different operating systems?

springmeyer · 2017-11-29T02:19:22Z

@mapsam, nice debugging. As an extra assurance that the numbers are changing at the JS conversion I recommend adding a std::setprecision call to your std::clog call. Set the precision higher so that you can see more digits after the decimal (equal to the JS). What I’d look for is that even with higher display precision the ‘ResObjDistance’ output is still different.

mapsam · 2017-11-29T17:26:54Z

Nice @springmeyer - here's the output with std::setprecision(20):

tilecoords:     601, 258
lnglat:         -87.796286344528198242, 41.867499528053855329
meters:         9.4363568893436244878
ResObjDistance: 9.4363568893436244878

Not sure how to read this, though.

springmeyer · 2017-11-29T18:08:18Z

@mapsam - thanks for posting that. So that helps show that the precision output was misleading because it was rounding when the C++ number was printed. So:

ResObjDistance: 9.43636

Is actually:

ResObjDistance: 9.4363568893436244878

Which look identical when printed (other than the precision):

9.4363568893436244878 # C++
9.436356889343624 # JS

Am I following along right?

mapsam · 2017-11-29T23:36:47Z

@springmeyer yep, that's right. I think it'll be best to just make sure our tests are checking a rounded number, considering anything beyond 4 decimal points is in the micrometers of distance.

I tried to recreate the priority queue sorting differences in a docker container (ubuntu:16.04) without any luck. The sort results end up the same. Perhaps I'm not using the proper versions?

~/mug/priority_queue_diffs$ clang++ -c -std=c++14 main.cpp && clang++ -o main main.o && ./main
d_a: 13.7984
d_c: 13.7984
d_b: 4.5493
d_d: 1.3325
~/mug/priority_queue_diffs$ docker run -it pqd
root@a01a48cfd288:/usr/local/src# g++ -c -std=c++14 main.cpp && g++ -o main main.o && ./main
d_a: 13.7984
d_c: 13.7984
d_b: 4.5493
d_d: 1.3325

springmeyer · 2017-11-29T23:44:26Z

Sounds good on checking a less precise number overall in the tests @mapsam As far as the priority queue issue which exact error is caused by that? Is it the error on travis where features properties are not matching the expected since they return in a different order on travis? If you could not replicate with a Dockerfile that is interesting. Could you post the Dockerfile?

…

On Nov 29, 2017, at 3:36 PM, Sam Matthews ***@***.***> wrote: @springmeyer yep, that's right. I think it'll be best to just make sure our tests are checking a rounded number, considering anything beyond 4 decimal points is in the micrometers of distance. I tried to recreate the priority queue sorting differences in a docker container without any luck. The sort results end up the same. Perhaps I'm not using the proper versions? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

mapsam · 2017-11-30T01:42:50Z

@springmeyer I've tried a few more attempts, and was able to recreate the error once, but I think I got lucky as I'm not able to get it compiled any longer. Now this branch includes a dockerfile you can build with docker build -t vtquery . and connect to it via docker run -it vtquery and then run make test inside, but I'm seeing some issues:

Error: /usr/local/src/lib/binding/vtquery.node: undefined symbol: __asan_report_store4

Not sure what's happening here, but perhaps I've set up the dockerfile incorrectly?

springmeyer · 2017-11-30T01:48:53Z

Remove the fsanitize=address from the Dockerfile and that error should go away. That worked for gzip-hpp but needs more tooling to work with a node-addon.

…

On Nov 29, 2017, at 5:42 PM, Sam Matthews ***@***.***> wrote: @springmeyer I've tried a few more attempts, and was able to recreate the error once, but I think I got lucky as I'm not able to get it compiled any longer. Now this branch includes a dockerfile you can build with docker build -t vtquery . and connect to it via docker run -it vtquery and then run make test inside, but I'm seeing some issues: Error: /usr/local/src/lib/binding/vtquery.node: undefined symbol: __asan_report_store4 Not sure what's happening here, but perhaps I've set up the dockerfile incorrectly? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

mapsam · 2017-11-30T05:48:48Z

Lovely, that did it @springmeyer. Thanks for the 👀! Now back to the investigation.

mapsam · 2017-11-30T19:55:47Z

I've removed the test for the time being and opened a ticket #44 to remind us of this in the future.

mapsam · 2017-12-01T18:37:33Z

Failing tests are due to travis regressions which have been ticketed in mapbox/node-cpp-skel#93

mapsam · 2017-12-01T18:38:01Z

Thanks @flippmoke and @springmeyer for the help here!

springmeyer · 2017-12-01T21:44:40Z

epic PR @mapsam 🙌

mapsam added 5 commits November 16, 2017 10:12

add vector-tile 2.x branch

1e4d116

integrate vector-tile, apply visitors, sort during

93b190b

move

a8715a1

0.0 conditional, priority queue

90beb6f

initialize empty pqueue

4e3ca66

mapsam commented Nov 22, 2017

View reviewed changes

mapsam added 3 commits November 22, 2017 10:13

back()

d8f9058

radius 0.0 tests, update README.md

5e2e943

tidy and format

b37d0bd

const reference fix

727cdac

mapsam mentioned this pull request Nov 22, 2017

Check for "within" results from closest_point #41

Closed

2 tasks

use spatial algorithms dev release

105f1d6

This was referenced Nov 27, 2017

Disable sort option #34

Open

Efficiency recommendations #30

Closed

mapsam mentioned this pull request Nov 27, 2017

optimizations (now and future) #22

Closed

12 tasks

reverse conditionals

144339c

mapsam mentioned this pull request Nov 27, 2017

radius=0 leads to zero results with PIP #36

Closed

mapsam added 2 commits November 27, 2017 12:47

quick format fix

28ba72a

allow 0.0 to be added always

5479c90

This was referenced Nov 27, 2017

Use vector-tile for geometry/property handling #25

Closed

Some code comments #40

Closed

mapsam changed the title ~~[WIP] Vector tile 2.x, sort during~~ Vector tile 2.x, sort during Nov 27, 2017

mapsam requested a review from springmeyer November 27, 2017 22:51

springmeyer approved these changes Nov 28, 2017

View reviewed changes

mapsam added 2 commits November 28, 2017 16:08

checkClose function

a0bb76d

more checkClose

7b79a90

add dockerfile

b080f21

remove sanitizers

aa0da23

mapsam mentioned this pull request Nov 30, 2017

regular results when distance values are the exact same #44

Closed

remove test

f5d447a

mapsam added 2 commits November 30, 2017 12:39

move Dockerfile into tmp folder

aa3feac

add LSAN flags

c793d31

mapsam merged commit 0a5423a into master Dec 1, 2017

mapsam deleted the vector-tile-2x branch December 1, 2017 18:38

mapsam mentioned this pull request Dec 1, 2017

Simplify geometry handling switch statement #45

Merged

Comments

Conversation

mapsam commented Nov 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changed?

benchmarks

Uh oh!

mapsam Nov 22, 2017

Choose a reason for hiding this comment

Uh oh!

mapsam Nov 27, 2017

Choose a reason for hiding this comment

Uh oh!

mapsam Nov 22, 2017

Choose a reason for hiding this comment

Uh oh!

mapsam Nov 27, 2017

Choose a reason for hiding this comment

Uh oh!

mapsam commented Nov 22, 2017

Uh oh!

mapsam commented Nov 27, 2017

Uh oh!

mapsam commented Nov 27, 2017

Uh oh!

springmeyer commented Nov 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

springmeyer left a comment

Choose a reason for hiding this comment

Uh oh!

mapsam commented Nov 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

springmeyer commented Nov 28, 2017

Uh oh!

mapsam commented Nov 29, 2017

Uh oh!

springmeyer commented Nov 29, 2017 via email

Uh oh!

mapsam commented Nov 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

springmeyer commented Nov 29, 2017

Uh oh!

mapsam commented Nov 29, 2017

Uh oh!

mapsam commented Nov 29, 2017

Uh oh!

springmeyer commented Nov 29, 2017

Uh oh!

mapsam commented Nov 29, 2017

Uh oh!

springmeyer commented Nov 29, 2017

Uh oh!

mapsam commented Nov 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

springmeyer commented Nov 29, 2017 via email

Uh oh!

mapsam commented Nov 30, 2017

Uh oh!

springmeyer commented Nov 30, 2017 via email

Uh oh!

mapsam commented Nov 30, 2017

Uh oh!

mapsam commented Nov 30, 2017

Uh oh!

mapsam commented Dec 1, 2017

Uh oh!

mapsam commented Dec 1, 2017

Uh oh!

springmeyer commented Dec 1, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

mapsam commented Nov 22, 2017 •

edited

Loading

springmeyer commented Nov 28, 2017 •

edited

Loading

mapsam commented Nov 28, 2017 •

edited

Loading

mapsam commented Nov 29, 2017 •

edited

Loading

mapsam commented Nov 29, 2017 •

edited

Loading