You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tinkerpop.apache.org by GitBox <gi...@apache.org> on 2021/03/10 16:15:42 UTC
[GitHub] [tinkerpop] FlorianHockmann opened a new pull request #1403: TINKERPOP-2472 GraphBinary for Gremlin.Net
FlorianHockmann opened a new pull request #1403:
URL: https://github.com/apache/tinkerpop/pull/1403
https://issues.apache.org/jira/browse/TINKERPOP-2472
The PR is not completely done yet as I haven't written any docs so far but I wanted to open the PR already since the implementation itself is already ready for review.
The test suite is still running with GraphSON 3 as that is the default serialization format. I looked into whether we can execute the Gherkin tests with different serialization formats by default but a lot of the Gherkin logic is static which makes it hard to make this configurable. Given how big the PR already is, I suggest that we keep it as is for now and change the configuration to GraphBinary manually to execute the tests locally when we make non-trivial changes to GraphBinary.
I also haven't implemented serializers for `Metrics` and `TraversalMetrics`. Not sure if we need those in the first version as they are also not supported for GraphSON. So, I would prefer to add them with a follow-up PR.
# Performance
## Benchmarks with BenchmarkDotNet
The PR also adds a simple benchmark project with initial benchmarks to compare GraphBinary against GraphSON 3. I created two test cases for the read path and two for the write path, one of each is for a very small message and one for a bigger message.
The results show a **speed improvement** compared to GraphSON 3 of roughly 1/3 for both deserialization and serialization.
But they also show that the GraphBinary implementation allocates more memory (by a factor of 3-4) during deserialization than GraphSON 3. This is something that we could improve in the future ([`Span`](https://docs.microsoft.com/en-us/archive/msdn-magazine/2018/january/csharp-all-about-span-exploring-a-new-net-mainstay) could potentially help here), but I think that it shouldn't stop us from releasing GraphBinary in Gremlin.Net.
Detailed results (taken on my laptop):
``` ini
BenchmarkDotNet=v0.12.1, OS=Windows 10.0.19041.804 (2004/?/20H1)
Intel Core i7-10850H CPU 2.70GHz, 1 CPU, 12 logical and 6 physical cores
.NET Core SDK=5.0.100
[Host] : .NET Core 3.1.9 (CoreCLR 4.700.20.47201, CoreFX 4.700.20.47203), X64 RyuJIT
Job-JJMOFB : .NET Core 3.1.9 (CoreCLR 4.700.20.47201, CoreFX 4.700.20.47203), X64 RyuJIT
IterationCount=5 LaunchCount=1 WarmupCount=3
```
| Method | Mean | Error | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|-------------------------------------- |-----------:|-----------:|----------:|---------:|--------:|------:|----------:|
| TestWriteEmptyBytecodeBinary | 4.114 μs | 0.1149 μs | 0.0298 μs | 0.3357 | - | - | 2.1 KB |
| TestWriteEmptyBytecodeGraphSON3 | 5.503 μs | 0.1823 μs | 0.0473 μs | 0.5417 | - | - | 3.33 KB |
| TestWriteBytecodeBinary | 22.241 μs | 0.3235 μs | 0.0840 μs | 1.3733 | - | - | 8.45 KB |
| TestWriteBytecodeGraphSON3 | 35.849 μs | 0.6938 μs | 0.1074 μs | 2.9297 | - | - | 18.27 KB |
| TestReadSmallResponseMessageBinary | 5.706 μs | 0.3858 μs | 0.0597 μs | 1.4954 | 0.0076 | - | 9.19 KB |
| TestReadSmallResponseMessageGraphSON3 | 10.025 μs | 0.9566 μs | 0.2484 μs | 0.5493 | - | - | 3.39 KB |
| TestReadBigResponseMessageBinary | 622.829 μs | 10.1589 μs | 1.5721 μs | 140.6250 | 34.1797 | - | 865.06 KB |
| TestReadBigResponseMessageGraphSON3 | 776.591 μs | 28.7843 μs | 4.4544 μs | 36.1328 | 11.7188 | - | 224.24 KB |
## Gherkin Tests
I also observed speed improvements for our Gherkin test suite. The tests take on average 5.0 seconds with GraphSON 3 and 3.8 seconds with GraphBinary. This shows of course not only the speed improvement from the .NET GraphBinary implementation but also includes the faster Java GraphBinary implementation.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette commented on pull request #1403: TINKERPOP-2472 GraphBinary for Gremlin.Net
Posted by GitBox <gi...@apache.org>.
spmallette commented on pull request #1403:
URL: https://github.com/apache/tinkerpop/pull/1403#issuecomment-800159590
> Given how big the PR already is, I suggest that we keep it as is for now and change the configuration to GraphBinary manually to execute the tests locally when we make non-trivial changes to GraphBinary.
I'm fine with that for now, but we really should have the GLV tests run with both serializers on each build. Please create a follow-on issue in JIRA for it and it can be some we return to later.
> I also haven't implemented serializers for Metrics and TraversalMetrics. Not sure if we need those in the first version as they are also not supported for GraphSON. So, I would prefer to add them with a follow-up PR.
GraphSON supports metrics: https://tinkerpop.apache.org/docs/3.4.10/dev/io/#_metrics_2 but if you just want to get in what you have that's fine. I presume we would add Upgrade Documentation and Reference Documentation adjustments when that happens?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette commented on pull request #1403: TINKERPOP-2472 GraphBinary for Gremlin.Net
Posted by GitBox <gi...@apache.org>.
spmallette commented on pull request #1403:
URL: https://github.com/apache/tinkerpop/pull/1403#issuecomment-812490470
Generally speaking this looks good to me. Just needs a CHANGELOG entry. VOTE +1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] FlorianHockmann merged pull request #1403: TINKERPOP-2472 GraphBinary for Gremlin.Net
Posted by GitBox <gi...@apache.org>.
FlorianHockmann merged pull request #1403:
URL: https://github.com/apache/tinkerpop/pull/1403
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] FlorianHockmann commented on pull request #1403: TINKERPOP-2472 GraphBinary for Gremlin.Net
Posted by GitBox <gi...@apache.org>.
FlorianHockmann commented on pull request #1403:
URL: https://github.com/apache/tinkerpop/pull/1403#issuecomment-805903141
I managed to configure the Gherkin tests to now test GraphBinary as well as GraphSON 3 so we have that already covered in this PR. I wanted to check the added docs but unfortunately I cannot build the docs right now on my machine. I think this shouldn't stop the review though.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] FlorianHockmann commented on pull request #1403: TINKERPOP-2472 GraphBinary for Gremlin.Net
Posted by GitBox <gi...@apache.org>.
FlorianHockmann commented on pull request #1403:
URL: https://github.com/apache/tinkerpop/pull/1403#issuecomment-801037619
> Please create a follow-on issue in JIRA for it and it can be some we return to later.
Done: https://issues.apache.org/jira/browse/TINKERPOP-2536
> GraphSON supports metrics
GraphSON does in general but I don't think that we have ever implemented support for them in Gremlin.Net. I just tried a `Profile()` traversal and get an exception: `Deserializer for "g:TraversalMetrics" not found`.
> I presume we would add Upgrade Documentation and Reference Documentation adjustments when that happens?
I would have added them now as GraphBinary is ready to be used with this PR from my side, but if you think that we definitely need support for metrics, then I can also add that first.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] FlorianHockmann commented on pull request #1403: TINKERPOP-2472 GraphBinary for Gremlin.Net
Posted by GitBox <gi...@apache.org>.
FlorianHockmann commented on pull request #1403:
URL: https://github.com/apache/tinkerpop/pull/1403#issuecomment-814085505
> Just needs a CHANGELOG entry.
Oh right, I seem to always forget the CHANGELOG when I add something to the upgrade docs. I've added CHANGELOG entries for the missing .NET contributions for 3.5.0.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] FlorianHockmann commented on pull request #1403: TINKERPOP-2472 GraphBinary for Gremlin.Net
Posted by GitBox <gi...@apache.org>.
FlorianHockmann commented on pull request #1403:
URL: https://github.com/apache/tinkerpop/pull/1403#issuecomment-811083605
I've fixed the problem with building docs on my machine (CTR commit will follow soon) and reviewed the generated docs. So, everything is good to go here from my side.
I'll merge this at the beginning of next week unless someone needs more time to review this. (I want to give more time for the review given the size of this PR.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org