{"action":"created","comment":{"url":"https://api.github.com/repos/google/flatbuffers/pulls/comments/118298314","pull_request_review_id":40076711,"id":118298314,"diff_hunk":"@@ -23,49 +23,233 @@\n #include \"grpc++/support/byte_buffer.h\"\n #include \"grpc/byte_buffer_reader.h\"\n \n+namespace flatbuffers {\n+namespace grpc {\n+\n+enum AddRef { ADD_REF };\n+enum StealRef { STEAL_REF };\n+\n+// Message is a typed wrapper around a buffer that manages the underlying\n+// `grpc_slice` and also provides flatbuffers-specific helpers such as `Verify`\n+// and `GetRoot`.","path":"include/flatbuffers/grpc.h","position":12,"original_position":12,"commit_id":"701ac1aa14bbef07a0c346426e2ef308add9ba19","original_commit_id":"701ac1aa14bbef07a0c346426e2ef308add9ba19","user":{"login":"llchan","id":51099,"avatar_url":"https://avatars1.githubusercontent.com/u/51099?v=3","gravatar_id":"","url":"https://api.github.com/users/llchan","html_url":"https://github.com/llchan","followers_url":"https://api.github.com/users/llchan/followers","following_url":"https://api.github.com/users/llchan/following{/other_user}","gists_url":"https://api.github.com/users/llchan/gists{/gist_id}","starred_url":"https://api.github.com/users/llchan/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/llchan/subscriptions","organizations_url":"https://api.github.com/users/llchan/orgs","repos_url":"https://api.github.com/users/llchan/repos","events_url":"https://api.github.com/users/llchan/events{/privacy}","received_events_url":"https://api.github.com/users/llchan/received_events","type":"User","site_admin":false},"body":"Will do, thanks for the suggestion","created_at":"2017-05-24T16:17:42Z","updated_at":"2017-05-24T16:17:42Z","html_url":"https://github.com/google/flatbuffers/pull/4310#discussion_r118298314","pull_request_url":"https://api.github.com/repos/google/flatbuffers/pulls/4310","_links":{"self":{"href":"https://api.github.com/repos/google/flatbuffers/pulls/comments/118298314"},"html":{"href":"https://github.com/google/flatbuffers/pull/4310#discussion_r118298314"},"pull_request":{"href":"https://api.github.com/repos/google/flatbuffers/pulls/4310"}}},"pull_request":{"url":"https://api.github.com/repos/google/flatbuffers/pulls/4310","id":120240996,"html_url":"https://github.com/google/flatbuffers/pull/4310","diff_url":"https://github.com/google/flatbuffers/pull/4310.diff","patch_url":"https://github.com/google/flatbuffers/pull/4310.patch","issue_url":"https://api.github.com/repos/google/flatbuffers/issues/4310","number":4310,"state":"open","locked":false,"title":"[C++] Improve flatbuffers + gRPC integration","user":{"login":"llchan","id":51099,"avatar_url":"https://avatars1.githubusercontent.com/u/51099?v=3","gravatar_id":"","url":"https://api.github.com/users/llchan","html_url":"https://github.com/llchan","followers_url":"https://api.github.com/users/llchan/followers","following_url":"https://api.github.com/users/llchan/following{/other_user}","gists_url":"https://api.github.com/users/llchan/gists{/gist_id}","starred_url":"https://api.github.com/users/llchan/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/llchan/subscriptions","organizations_url":"https://api.github.com/users/llchan/orgs","repos_url":"https://api.github.com/users/llchan/repos","events_url":"https://api.github.com/users/llchan/events{/privacy}","received_events_url":"https://api.github.com/users/llchan/received_events","type":"User","site_admin":false},"body":"This PR is a work in progress that stems from some experiments surrounding #4306.\r\n\r\nWith this PR, flatbuffers can now build messages in gRPC's reference counted `grpc_slice` buffers, allowing us to transfer ownership to gRPC without a copy. This simplifies some of the bookkeeping surrounding buffer ownership and timing, and also reduces the amount of redundant copying (especially when a message is broadcast to many clients).\r\n\r\nNew API:\r\n- `flatbuffers::BufferRef<T>` becomes `flatbuffers::grpc::Message<T>`, which is a buffer with memory managed by a `grpc_slice`. Most of the reading API remains the same (i.e. `Verify` and `GetRoot`), so other than a type change most existing code should work as-is.\r\n- `flatbuffers::grpc::MessageBuilder` is used instead of `flatbuffers::FlatBufferBuilder` . This is a collection of extra functionality bolted onto `FlatBufferBuilder`:\r\n    - It forces the use of a `flatbuffers::grpc::SliceAllocator`, which does the `grpc_slice` allocation\r\n    - It adds `GetMessage<T>()` and `ReleaseMessage<T>()` methods, which retrieve `Message<T>` objects backed by subslices of the underlying buffer (refcounts are incremented accordingly). The release variant detaches the builder from the message so that the builder can be reused safely without mangling in-flight data.\r\n    - When the building is done and we want to finalize the message, instead of constructing a `BufferRef<T>` from a ptr/size into the buffer, we call `GetMessage<T>()` or `ReleaseMessage<T>()` to obtain a properly reference-counted message that we can then write back as the rpc response.\r\n    - The rest of the `FlatBufferBuilder` methods remain unchanged, so most of the the actual message building code should remain unchanged. \r\n\r\nA few changes to existing code:\r\n- I inherit from FlatBufferBuilder to have access to the underlying buffer while retaining most of the builder methods. To facilitate this, the `final` was removed and `private` was converted to `protected`. There may be a better way to structure this.\r\n- The `vector_downward::release` code was incorrect. The deleter was always using the `simple_allocator::deallocate` instead of the fully derived `deallocate`. This is fixed with a lambda, but depending on compatibility requirements we may need to do something else.\r\n- I exposed the underlying `uint8_t *buf_` pointer of `vector_downward` via a `buf()` method, so we can look up the `grpc_slice` corresponding to the current buffer. If there are objections we could potentially do this by checking which memory region a pointer belongs to, but I don't see any harm in exposing it.\r\n\r\nA few warts:\r\n- I originally had the `SliceAllocator` as a member and not a base class of `MessageBuilder`, but the initialization order wasn't right and the `FlatBufferBuilder` would try to use it before it was ready. We can probably refactor something to make this better. Relatedly, there's an extraneous `simple_allocator` member in the FlatBufferBuilder that doesnt need to be there.\r\n- I marked the `SliceAllocator` members as `mutable` because there are some const allocators floating around. Why are they const? I didn't change anything yet but I think we can just pass around non-const allocators, since it's not unreasonable that they might keep state that `allocate`/`deallocate` could mutate.\r\n\r\nAnyways, this branch passes the `grpctest` program, but more work needs to be done on the testing front, and we should get more eyes on this to verify the reference counting and such. I'm very much open to feedback and we can iterate on this until everyone's happy with it, so let me know what you think!","created_at":"2017-05-12T03:09:19Z","updated_at":"2017-05-24T16:17:42Z","closed_at":null,"merged_at":null,"merge_commit_sha":"edd8250ce292c49a8ce31931ef1e6bf74d77439c","assignee":null,"assignees":[],"requested_reviewers":[],"milestone":null,"commits_url":"https://api.github.com/repos/google/flatbuffers/pulls/4310/commits","review_comments_url":"https://api.github.com/repos/google/flatbuffers/pulls/4310/comments","review_comment_url":"https://api.github.com/repos/google/flatbuffers/pulls/comments{/number}","comments_url":"https://api.github.com/repos/google/flatbuffers/issues/4310/comments","statuses_url":"https://api.github.com/repos/google/flatbuffers/statuses/701ac1aa14bbef07a0c346426e2ef308add9ba19","head":{"label":"llchan:grpc-buffers","ref":"grpc-buffers","sha":"701ac1aa14bbef07a0c346426e2ef308add9ba19","user":{"login":"llchan","id":51099,"avatar_url":"https://avatars1.githubusercontent.com/u/51099?v=3","gravatar_id":"","url":"https://api.github.com/users/llchan","html_url":"https://github.com/llchan","followers_url":"https://api.github.com/users/llchan/followers","following_url":"https://api.github.com/users/llchan/following{/other_user}","gists_url":"https://api.github.com/users/llchan/gists{/gist_id}","starred_url":"https://api.github.com/users/llchan/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/llchan/subscriptions","organizations_url":"https://api.github.com/users/llchan/orgs","repos_url":"https://api.github.com/users/llchan/repos","events_url":"https://api.github.com/users/llchan/events{/privacy}","received_events_url":"https://api.github.com/users/llchan/received_events","type":"User","site_admin":false},"repo":{"id":73887384,"name":"flatbuffers","full_name":"llchan/flatbuffers","owner":{"login":"llchan","id":51099,"avatar_url":"https://avatars1.githubusercontent.com/u/51099?v=3","gravatar_id":"","url":"https://api.github.com/users/llchan","html_url":"https://github.com/llchan","followers_url":"https://api.github.com/users/llchan/followers","following_url":"https://api.github.com/users/llchan/following{/other_user}","gists_url":"https://api.github.com/users/llchan/gists{/gist_id}","starred_url":"https://api.github.com/users/llchan/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/llchan/subscriptions","organizations_url":"https://api.github.com/users/llchan/orgs","repos_url":"https://api.github.com/users/llchan/repos","events_url":"https://api.github.com/users/llchan/events{/privacy}","received_events_url":"https://api.github.com/users/llchan/received_events","type":"User","site_admin":false},"private":false,"html_url":"https://github.com/llchan/flatbuffers","description":"Memory Efficient Serialization Library","fork":true,"url":"https://api.github.com/repos/llchan/flatbuffers","forks_url":"https://api.github.com/repos/llchan/flatbuffers/forks","keys_url":"https://api.github.com/repos/llchan/flatbuffers/keys{/key_id}","collaborators_url":"https://api.github.com/repos/llchan/flatbuffers/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/llchan/flatbuffers/teams","hooks_url":"https://api.github.com/repos/llchan/flatbuffers/hooks","issue_events_url":"https://api.github.com/repos/llchan/flatbuffers/issues/events{/number}","events_url":"https://api.github.com/repos/llchan/flatbuffers/events","assignees_url":"https://api.github.com/repos/llchan/flatbuffers/assignees{/user}","branches_url":"https://api.github.com/repos/llchan/flatbuffers/branches{/branch}","tags_url":"https://api.github.com/repos/llchan/flatbuffers/tags","blobs_url":"https://api.github.com/repos/llchan/flatbuffers/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/llchan/flatbuffers/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/llchan/flatbuffers/git/refs{/sha}","trees_url":"https://api.github.com/repos/llchan/flatbuffers/git/trees{/sha}","statuses_url":"https://api.github.com/repos/llchan/flatbuffers/statuses/{sha}","languages_url":"https://api.github.com/repos/llchan/flatbuffers/languages","stargazers_url":"https://api.github.com/repos/llchan/flatbuffers/stargazers","contributors_url":"https://api.github.com/repos/llchan/flatbuffers/contributors","subscribers_url":"https://api.github.com/repos/llchan/flatbuffers/subscribers","subscription_url":"https://api.github.com/repos/llchan/flatbuffers/subscription","commits_url":"https://api.github.com/repos/llchan/flatbuffers/commits{/sha}","git_commits_url":"https://api.github.com/repos/llchan/flatbuffers/git/commits{/sha}","comments_url":"https://api.github.com/repos/llchan/flatbuffers/comments{/number}","issue_comment_url":"https://api.github.com/repos/llchan/flatbuffers/issues/comments{/number}","contents_url":"https://api.github.com/repos/llchan/flatbuffers/contents/{+path}","compare_url":"https://api.github.com/repos/llchan/flatbuffers/compare/{base}...{head}","merges_url":"https://api.github.com/repos/llchan/flatbuffers/merges","archive_url":"https://api.github.com/repos/llchan/flatbuffers/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/llchan/flatbuffers/downloads","issues_url":"https://api.github.com/repos/llchan/flatbuffers/issues{/number}","pulls_url":"https://api.github.com/repos/llchan/flatbuffers/pulls{/number}","milestones_url":"https://api.github.com/repos/llchan/flatbuffers/milestones{/number}","notifications_url":"https://api.github.com/repos/llchan/flatbuffers/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/llchan/flatbuffers/labels{/name}","releases_url":"https://api.github.com/repos/llchan/flatbuffers/releases{/id}","deployments_url":"https://api.github.com/repos/llchan/flatbuffers/deployments","created_at":"2016-11-16T05:30:50Z","updated_at":"2016-11-16T05:30:51Z","pushed_at":"2017-05-23T00:36:22Z","git_url":"git://github.com/llchan/flatbuffers.git","ssh_url":"git@github.com:llchan/flatbuffers.git","clone_url":"https://github.com/llchan/flatbuffers.git","svn_url":"https://github.com/llchan/flatbuffers","homepage":"http://google.github.io/flatbuffers/","size":4341,"stargazers_count":0,"watchers_count":0,"language":"C++","has_issues":false,"has_projects":true,"has_downloads":true,"has_wiki":false,"has_pages":false,"forks_count":0,"mirror_url":null,"open_issues_count":0,"forks":0,"open_issues":0,"watchers":0,"default_branch":"master"}},"base":{"label":"google:master","ref":"master","sha":"c559eb451e7783b72c44e27db375158edaa498b7","user":{"login":"google","id":1342004,"avatar_url":"https://avatars2.githubusercontent.com/u/1342004?v=3","gravatar_id":"","url":"https://api.github.com/users/google","html_url":"https://github.com/google","followers_url":"https://api.github.com/users/google/followers","following_url":"https://api.github.com/users/google/following{/other_user}","gists_url":"https://api.github.com/users/google/gists{/gist_id}","starred_url":"https://api.github.com/users/google/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/google/subscriptions","organizations_url":"https://api.github.com/users/google/orgs","repos_url":"https://api.github.com/users/google/repos","events_url":"https://api.github.com/users/google/events{/privacy}","received_events_url":"https://api.github.com/users/google/received_events","type":"Organization","site_admin":false},"repo":{"id":19953044,"name":"flatbuffers","full_name":"google/flatbuffers","owner":{"login":"google","id":1342004,"avatar_url":"https://avatars2.githubusercontent.com/u/1342004?v=3","gravatar_id":"","url":"https://api.github.com/users/google","html_url":"https://github.com/google","followers_url":"https://api.github.com/users/google/followers","following_url":"https://api.github.com/users/google/following{/other_user}","gists_url":"https://api.github.com/users/google/gists{/gist_id}","starred_url":"https://api.github.com/users/google/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/google/subscriptions","organizations_url":"https://api.github.com/users/google/orgs","repos_url":"https://api.github.com/users/google/repos","events_url":"https://api.github.com/users/google/events{/privacy}","received_events_url":"https://api.github.com/users/google/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/google/flatbuffers","description":"Memory Efficient Serialization Library","fork":false,"url":"https://api.github.com/repos/google/flatbuffers","forks_url":"https://api.github.com/repos/google/flatbuffers/forks","keys_url":"https://api.github.com/repos/google/flatbuffers/keys{/key_id}","collaborators_url":"https://api.github.com/repos/google/flatbuffers/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/google/flatbuffers/teams","hooks_url":"https://api.github.com/repos/google/flatbuffers/hooks","issue_events_url":"https://api.github.com/repos/google/flatbuffers/issues/events{/number}","events_url":"https://api.github.com/repos/google/flatbuffers/events","assignees_url":"https://api.github.com/repos/google/flatbuffers/assignees{/user}","branches_url":"https://api.github.com/repos/google/flatbuffers/branches{/branch}","tags_url":"https://api.github.com/repos/google/flatbuffers/tags","blobs_url":"https://api.github.com/repos/google/flatbuffers/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/google/flatbuffers/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/google/flatbuffers/git/refs{/sha}","trees_url":"https://api.github.com/repos/google/flatbuffers/git/trees{/sha}","statuses_url":"https://api.github.com/repos/google/flatbuffers/statuses/{sha}","languages_url":"https://api.github.com/repos/google/flatbuffers/languages","stargazers_url":"https://api.github.com/repos/google/flatbuffers/stargazers","contributors_url":"https://api.github.com/repos/google/flatbuffers/contributors","subscribers_url":"https://api.github.com/repos/google/flatbuffers/subscribers","subscription_url":"https://api.github.com/repos/google/flatbuffers/subscription","commits_url":"https://api.github.com/repos/google/flatbuffers/commits{/sha}","git_commits_url":"https://api.github.com/repos/google/flatbuffers/git/commits{/sha}","comments_url":"https://api.github.com/repos/google/flatbuffers/comments{/number}","issue_comment_url":"https://api.github.com/repos/google/flatbuffers/issues/comments{/number}","contents_url":"https://api.github.com/repos/google/flatbuffers/contents/{+path}","compare_url":"https://api.github.com/repos/google/flatbuffers/compare/{base}...{head}","merges_url":"https://api.github.com/repos/google/flatbuffers/merges","archive_url":"https://api.github.com/repos/google/flatbuffers/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/google/flatbuffers/downloads","issues_url":"https://api.github.com/repos/google/flatbuffers/issues{/number}","pulls_url":"https://api.github.com/repos/google/flatbuffers/pulls{/number}","milestones_url":"https://api.github.com/repos/google/flatbuffers/milestones{/number}","notifications_url":"https://api.github.com/repos/google/flatbuffers/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/google/flatbuffers/labels{/name}","releases_url":"https://api.github.com/repos/google/flatbuffers/releases{/id}","deployments_url":"https://api.github.com/repos/google/flatbuffers/deployments","created_at":"2014-05-19T18:33:01Z","updated_at":"2017-05-24T13:44:30Z","pushed_at":"2017-05-23T00:36:24Z","git_url":"git://github.com/google/flatbuffers.git","ssh_url":"git@github.com:google/flatbuffers.git","clone_url":"https://github.com/google/flatbuffers.git","svn_url":"https://github.com/google/flatbuffers","homepage":"http://google.github.io/flatbuffers/","size":4484,"stargazers_count":7822,"watchers_count":7822,"language":"C++","has_issues":true,"has_projects":true,"has_downloads":true,"has_wiki":false,"has_pages":true,"forks_count":1184,"mirror_url":null,"open_issues_count":186,"forks":1184,"open_issues":186,"watchers":7822,"default_branch":"master"}},"_links":{"self":{"href":"https://api.github.com/repos/google/flatbuffers/pulls/4310"},"html":{"href":"https://github.com/google/flatbuffers/pull/4310"},"issue":{"href":"https://api.github.com/repos/google/flatbuffers/issues/4310"},"comments":{"href":"https://api.github.com/repos/google/flatbuffers/issues/4310/comments"},"review_comments":{"href":"https://api.github.com/repos/google/flatbuffers/pulls/4310/comments"},"review_comment":{"href":"https://api.github.com/repos/google/flatbuffers/pulls/comments{/number}"},"commits":{"href":"https://api.github.com/repos/google/flatbuffers/pulls/4310/commits"},"statuses":{"href":"https://api.github.com/repos/google/flatbuffers/statuses/701ac1aa14bbef07a0c346426e2ef308add9ba19"}}}}
{"actor":{"display_login":"llchan"}}