Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/protobuf output #1273

Closed
wants to merge 58 commits into from
Closed

Feature/protobuf output #1273

wants to merge 58 commits into from

Conversation

DennisOSRM
Copy link
Collaborator

Adds a protobuf descriptor and supersedes #1161.

TODO:

  • rebase on develop
  • fix protoc call on Windows
  • add protobuffer output for
    • viaroute
    • distance table
    • locate
    • nearest
  • remove camel case from response.proto
  • add test to check for correct output
    • viaroute
    • distance table
    • locate
    • nearest
  • make sure JSON and pbf field names are similar/same.
  • micro benchmarks for
    • speed
    • file size

@DennisOSRM
Copy link
Collaborator Author

Windows is rather developer unfriendly. Fixing the Protobuf issues is not straight-forward. I fear we have to bundle protoc.exe with the dependencies and point to it manually.

cc: @alex85k

@alex85k
Copy link
Contributor

alex85k commented Nov 17, 2014

@DennisOSRM : The newer dependencies you published at http://build.project-osrm.org/libs_osrm_Release_test.7z already have protoc.exe for protobuf 2.6. Building it from source would be really slow...

@DennisOSRM DennisOSRM force-pushed the feature/protobuf-output branch from bc04015 to 3b70649 Compare November 20, 2014 11:21
@DennisOSRM
Copy link
Collaborator Author

rebased onto develop branch after merge of the osmium parsing. Nearing completion here, too.

@alex85k
Copy link
Contributor

alex85k commented Nov 26, 2014

Newest protoc.exe is already in dependencies (together with protobuf v2.6.1), current path to it on appveyor is c:/projects/osrm/libs/bin/protoc.exe
CMake should find it automatically on next push.

@TheMarex
Copy link
Member

TheMarex commented Jan 2, 2015

Rebased on develop and push to my fork:
https://github.com/TheMarex/Project-OSRM/tree/feature/protobuf-output
I expect this will need some rework after #1292 is merged?

@DennisOSRM
Copy link
Collaborator Author

@TheMarex yep, once #1292 is merged, this is following suit.

Want to convert this to a feature branch in the main repo, and replace this PR?

@DennisOSRM DennisOSRM force-pushed the feature/protobuf-output branch from 3b70649 to 94b780a Compare January 7, 2015 12:32
@DennisOSRM
Copy link
Collaborator Author

rebased onto develop. Now that the common interface between libOSRM and the rest of the stack is a json_containerobject, the question is whether it makes sense to refactor the ProtoBuffer generation of the output into a renderer that traverses the container.

@DennisOSRM
Copy link
Collaborator Author

Note to self:

There's a hacky solution that doesn't break interface too much.

  • augment json_container with char array.
  • move char array into response when set, otherwise render JSON/XML into response.

@DennisOSRM
Copy link
Collaborator Author

Windows compile works fine now. Thanks @alex85k for providing protoc binaries.

@DennisOSRM
Copy link
Collaborator Author

@emiltin how hard is it to parse protobuffer encoded responses in cucumber tests?

@emiltin
Copy link
Contributor

emiltin commented Jan 7, 2015

Haven't tried it, but I guess it shouldn't be too hard. There are of course Ruby bindings for protobuf https://code.google.com/p/ruby-protobuf. Someone is also maintaining RSpec matchers that can be used with cucumber: https://github.com/connamara/protobuf_spec.

@DennisOSRM
Copy link
Collaborator Author

@emiltin thanks sounds easy enough. What do you think about checking the JSON as well the protobuffer encoded response with each query run during tests.

@emiltin
Copy link
Contributor

emiltin commented Jan 7, 2015

I think it would be better to have a set of tests that specifically exercise the protobuf response format, and have the rest of the tests use only JOSN (or protobuffer if that's faster, which I doubt).

@DennisOSRM
Copy link
Collaborator Author

yeah, that probably suffices.

@DennisOSRM DennisOSRM force-pushed the feature/protobuf-output branch from d329456 to 94be790 Compare January 8, 2015 16:06
@DennisOSRM
Copy link
Collaborator Author

basic implementation is more or less done. Now it's time to get this through the last lap, i.e. add tests and verify the implementation is complete.

@emiltin
Copy link
Contributor

emiltin commented Jan 9, 2015

what's the benefit of protobuf output - smaller size?

@TheMarex
Copy link
Member

TheMarex commented Jan 9, 2015

@emiltin Deserialization should also be faster. Should be interesting for people who use libOSRM and compute a lot of queries/minute.

@emiltin
Copy link
Contributor

emiltin commented Jan 10, 2015

i'm looking into adding tests for pbf output, but i'm getting empty outputs:

~/code/osrm-backend$ (feature/protobuf-output *) curl "http://127.0.0.1:5000/viaroute?loc=1.0,1.0&loc=1.0,1.0008990679362704&alt=false&instructions=true&output=json" -I
HTTP/1.0 200 OK
Content-Length: 558
Content-Type: application/json; charset=UTF-8
Content-Disposition: inline; filename="response.json"

~/code/osrm-backend$ (feature/protobuf-output *) curl "http://127.0.0.1:5000/viaroute?loc=1.0,1.0&loc=1.0,1.0008990679362704&alt=false&instructions=true&output=pbf" -I
HTTP/1.0 200 OK
Content-Length: 0
Content-Type: application/x-protobuf; charset=UTF-8
Content-Disposition: inline; filename="response.json"

it also seems odd that the filename includes ".json"?
am i doing something wrong?

@DennisOSRM
Copy link
Collaborator Author

@emiltin the code path is probably not correctly wired. I'll get to that Monday.

@emiltin
Copy link
Contributor

emiltin commented Jan 12, 2015

if you're using osrm as a lib, can you use the basic c++ structures as response format, ie bypass pbf/json?

@DennisOSRM
Copy link
Collaborator Author

The lib interface is called JSON container. It's an abstract data structure that behaves very similar to what JSON feels like.

@emiltin
Copy link
Contributor

emiltin commented Jan 12, 2015

ok. just wondering why you would want to use pbf if you use osrm as a lib. wouldn't the encoding/decoding be unnecessary?

@DennisOSRM
Copy link
Collaborator Author

yes, and no. You are right that it doesn't make too much sense when you use the lib. The use case is when you don't use the lib, but run requests over http.

@TheMarex
Copy link
Member

Ah we now expose the json container in the library interface as well? For some reason I thought it was still http::Reply there. Cool.

@DennisOSRM
Copy link
Collaborator Author

First benchmarks have come in. The numbers should be taken with a bit of caution but they indicate good file sizes:

uncompr.  compr. compressibility compr. file size over JSON
json   1.00 0.46 0.46 0.46
pbf 0.72 0.66 0.43 0.31

The numbers are relative to the respective JSON size. What we see is that the uncompressed protocol buffer encoded outputs are smaller and compress a little worse. This is expected and the overall compressed file size is still smaller by a good margin.

@emiltin 0384c21 should have proper output. Please note that it is binary encoded data and that it may appear as empty when printing to the console.

@emiltin
Copy link
Contributor

emiltin commented Jan 12, 2015

do you mean the compressed json is smaller (0.46) than both raw pbf (0.72) and compresed pbf (0.66)?

@DennisOSRM DennisOSRM force-pushed the feature/protobuf-output branch from a586346 to a384955 Compare February 24, 2015 07:58
@emiltin
Copy link
Contributor

emiltin commented Feb 24, 2015

did you regenerate the .pp.rb file?

@DennisOSRM
Copy link
Collaborator Author

Right, regenerated but there were no significant differences to the existing one.

@emiltin
Copy link
Contributor

emiltin commented Feb 24, 2015

the protobuffer documenation (google, not the gem) states:

Groups
Note that this feature is deprecated and should not be used when creating new message types – use nested message types instead.

so either the protobuffer definition uses these deprecated, or is somehow intepreted wrong?

@emiltin
Copy link
Contributor

emiltin commented Feb 24, 2015

response.pb.cc includes references to WIRETYPE_END_GROUP, which is causing exceptions at https://github.com/localshred/protobuf/blob/a5e6efcca4a8ac79bc21566411b3faeed8a07804/lib/protobuf/decoder.rb#L28.

could it be osmium using pbf groups?

@TheMarex
Copy link
Member

Dropping this PR because it only increases the surface of code to maintain. We should support JSON output only for simplicity and maintainability. If you are worried about data size of the response consider using node-osrm and compress the response. The compression of 0.31 of protobuf over compressed JSON 0.46 is not enough to warrant this feature.

@TheMarex TheMarex closed this Jul 31, 2015
@daniel-j-h daniel-j-h deleted the feature/protobuf-output branch February 22, 2017 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants