You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tinkerpop.apache.org by "jorgebay (GitHub)" <gi...@apache.org> on 2019/02/18 15:50:49 UTC
[GitHub] [tinkerpop] jorgebay opened pull request #1065:
TINKERPOP-2161 - GraphBinary: use a single buffer instead of allocators
Improve write path by reusing the same buffer.
For serializers this is an API change but given that GraphBinary was introduced recently, I think we can get away with it.
[ Full content available at: https://github.com/apache/tinkerpop/pull/1065 ]
This message was relayed via gitbox.apache.org for dev@tinkerpop.apache.org
[GitHub] [tinkerpop] jorgebay commented on issue #1065:
TINKERPOP-2161 - GraphBinary: use a single buffer instead of allocators
Posted by "jorgebay (GitHub)" <gi...@apache.org>.
I've added the CHANGELOG entries and removed the unused parameter from `ResponseMessageSerializer` (TINKERPOP-2153). I think this is ready to be reviewed.
VOTE +1
[ Full content available at: https://github.com/apache/tinkerpop/pull/1065 ]
This message was relayed via gitbox.apache.org for dev@tinkerpop.apache.org
[GitHub] [tinkerpop] jorgebay closed pull request #1065:
TINKERPOP-2161 - GraphBinary: use a single buffer instead of allocators
Posted by "jorgebay (GitHub)" <gi...@apache.org>.
[ pull request closed by jorgebay ]
[ Full content available at: https://github.com/apache/tinkerpop/pull/1065 ]
This message was relayed via gitbox.apache.org for dev@tinkerpop.apache.org
[GitHub] [tinkerpop] nastra commented on pull request #1065:
TINKERPOP-2161 - GraphBinary: use a single buffer instead of allocators
Posted by "nastra (GitHub)" <gi...@apache.org>.
should these tests be enabled again?
[ Full content available at: https://github.com/apache/tinkerpop/pull/1065 ]
This message was relayed via gitbox.apache.org for dev@tinkerpop.apache.org
[GitHub] [tinkerpop] spmallette commented on issue #1065:
TINKERPOP-2161 - GraphBinary: use a single buffer instead of allocators
Posted by "spmallette (GitHub)" <gi...@apache.org>.
looks fine though we usually reference the JIRA at the end of the description of the change:
```text
See: link:https://issues.apache.org/jira/browse/TINKERPOP-1774[TINKERPOP-1774]
```
[ Full content available at: https://github.com/apache/tinkerpop/pull/1065 ]
This message was relayed via gitbox.apache.org for dev@tinkerpop.apache.org
[GitHub] [tinkerpop] jorgebay commented on issue #1065:
TINKERPOP-2161 - GraphBinary: use a single buffer instead of allocators
Posted by "jorgebay (GitHub)" <gi...@apache.org>.
I've included more benchmarks in the project to compare GraphSON vs GraphBinary side by side, with more detail about different uses/types. The results from my machine are:
```
Benchmark Mode Cnt Score Error Units
SerializationBenchmark.testReadMessage1Binary thrpt 20 4933443.595 ± 108335.893 ops/s
SerializationBenchmark.testReadMessage1GraphSON thrpt 20 34318.763 ± 7782.995 ops/s
SerializationBenchmark.testReadMessage2Binary thrpt 20 974269.201 ± 27497.059 ops/s
SerializationBenchmark.testReadMessage2GraphSON thrpt 20 29928.189 ± 1187.881 ops/s
SerializationBenchmark.testWriteBytecodeBinary thrpt 20 932400.254 ± 24727.644 ops/s
SerializationBenchmark.testWriteBytecodeGraphSON thrpt 20 690590.285 ± 19508.398 ops/s
SerializationBenchmark.testWriteResponseBinary thrpt 20 2629038.176 ± 67510.162 ops/s
SerializationBenchmark.testWriteResponseGraphSON thrpt 20 652272.129 ± 20014.138 ops/s
```
These new results should be compared with the next group:
```
Benchmark Mode Cnt Score Error Units
GraphBinaryReaderWriterBenchmark.readBytecode1 thrpt 20 1400024.496 ± 64637.425 ops/s
GraphBinaryReaderWriterBenchmark.readBytecode2 thrpt 20 343833.159 ± 10609.128 ops/s
GraphBinaryReaderWriterBenchmark.writeBytecode1 thrpt 20 1857617.437 ± 100685.512 ops/s
GraphBinaryReaderWriterBenchmark.writeBytecode2 thrpt 20 431804.339 ± 24291.109 ops/s
```
```
Benchmark Mode Cnt Score Error Units
GraphSONMapperBenchmark.readBytecode1 thrpt 20 592331.268 ± 71423.780 ops/s
GraphSONMapperBenchmark.readBytecode2 thrpt 20 123595.490 ± 9640.191 ops/s
GraphSONMapperBenchmark.writeBytecode1 thrpt 20 1352647.895 ± 348521.676 ops/s
GraphSONMapperBenchmark.writeBytecode2 thrpt 20 371473.035 ± 35986.231 ops/s
```
In the process, I've found that `P` deserialization can be optimized on both GraphSON and GraphBinary, I've filed TINKERPOP-2166 for it.
[ Full content available at: https://github.com/apache/tinkerpop/pull/1065 ]
This message was relayed via gitbox.apache.org for dev@tinkerpop.apache.org
[GitHub] [tinkerpop] jorgebay commented on issue #1065:
TINKERPOP-2161 - GraphBinary: use a single buffer instead of allocators
Posted by "jorgebay (GitHub)" <gi...@apache.org>.
I'll include TINKERPOP-2153 changes in this ticket also, to include all breaking changes into one.
[ Full content available at: https://github.com/apache/tinkerpop/pull/1065 ]
This message was relayed via gitbox.apache.org for dev@tinkerpop.apache.org
[GitHub] [tinkerpop] jorgebay commented on pull request #1065:
TINKERPOP-2161 - GraphBinary: use a single buffer instead of allocators
Posted by "jorgebay (GitHub)" <gi...@apache.org>.
oh, good catch!
I'll fix it.
[ Full content available at: https://github.com/apache/tinkerpop/pull/1065 ]
This message was relayed via gitbox.apache.org for dev@tinkerpop.apache.org
[GitHub] [tinkerpop] jorgebay commented on issue #1065:
TINKERPOP-2161 - GraphBinary: use a single buffer instead of allocators
Posted by "jorgebay (GitHub)" <gi...@apache.org>.
I've added a sub-section to the upgrade docs, lmkwyt.
[ Full content available at: https://github.com/apache/tinkerpop/pull/1065 ]
This message was relayed via gitbox.apache.org for dev@tinkerpop.apache.org
[GitHub] [tinkerpop] spmallette commented on issue #1065:
TINKERPOP-2161 - GraphBinary: use a single buffer instead of allocators
Posted by "spmallette (GitHub)" <gi...@apache.org>.
Since there are minor API breaks I think that the upgrade docs could use a section to just describe what's changed and how to deal with the compilation issues that might be faced. I would think that it would be added as a sub-section to "Upgrading for Providers > Graph System Providers".
Other than that - VOTE +1
[ Full content available at: https://github.com/apache/tinkerpop/pull/1065 ]
This message was relayed via gitbox.apache.org for dev@tinkerpop.apache.org
[GitHub] [tinkerpop] spmallette commented on pull request #1065:
TINKERPOP-2161 - GraphBinary: use a single buffer instead of allocators
Posted by "spmallette (GitHub)" <gi...@apache.org>.
nit: no need to multi-line the changelog. it can just run on as needed on a single line.
[ Full content available at: https://github.com/apache/tinkerpop/pull/1065 ]
This message was relayed via gitbox.apache.org for dev@tinkerpop.apache.org