DL-45: DL should allow ByteBuf based API to avoid copying bytes array by sijie · Pull Request #151 · apache/distributedlog

sijie · 2017-08-22T19:12:58Z

Descriptions of the changes in this PR:

This change leverages the ByteBuf api introduced in bookkeeper 4.5.0. It will avoid copying bytes array between LogRecord and LogRecordSet/Entry, and avoid copying bytes from DL to BK client.

This change also bump the lz4 library to 1.3.0 to leverage the ByteBuffer binding.

- registerSuccessEvent and registerFailureEvent needs TimeUnit - change HashedWheelTimer to netty 4 - change channelFactory to eventLoopGroup

…version

sijie · 2017-08-22T19:14:27Z

/cc @leighst for review

This change doesn't include any benchmark result. I will have a subsequent change to have a micro-benchmark result to compare the serialization/deserialization performance.

eolivelli

Great improvement, basically it is the switch to pooled and reference counted bytebufs

eolivelli · 2017-08-23T06:44:24Z

distributedlog-protocol/src/main/java/org/apache/distributedlog/EnvelopedRecordSetReader.java

    }

+    @Override
+    protected void finalize() throws Throwable {


Just to be curious, why are you using finalize?
I think that finalize is bad and legacy mechanism in Java and could lead in general to problems

I know finalize is not a reliable and recommended way to release resource. However if I don't do finalize, there is no other way for me to release resources. or I have to change every classes that explicitly release the byte buf, this is going to a big change. also it is going to hard to change public structure like LogRecord to release references. it is going to break the API.

I will have a subsequent change to improve this. because I don't want to mix them in a big pull request.

+1 on this, seems like its not something we should do lightly

how are you planning to solve it?

hellostreaming

+1.
Thanks for the great work.

sijie · 2017-08-30T01:27:20Z

@leighst @mgodave can any of you review this as well?

leighst

LGTM

leighst · 2017-08-30T03:30:06Z

distributedlog-core/src/main/java/org/apache/distributedlog/EnvelopedEntry.java

-        EnvelopedEntry entry = new EnvelopedEntry(version, statsLogger);
-        entry.readFully(new DataInputStream(src));
-        return new ByteArrayInputStream(entry.getDecompressedPayload());
+    public static ByteBuf fromEnvlopedBuf(ByteBuf src, StatsLogger statsLogger)


typo: fromEnvlopedBuf

leighst · 2017-08-30T03:33:26Z

distributedlog-core/src/main/java/org/apache/distributedlog/EnvelopedEntry.java

+        }
+        int flags = src.readInt();
+        int codecCode = flags & COMPRESSION_CODEC_MASK;
+        int originDataLen = src.readInt();


do you mean "original"?

changed to 'compressed'

leighst · 2017-08-30T03:38:50Z

distributedlog-core/src/main/java/org/apache/distributedlog/EnvelopedEntryWriter.java

-        this.buffer = new Buffer(initialBufferSize * 6 / 5);
-        this.writer = new LogRecord.Writer(new DataOutputStream(buffer));
+        this.buffer = PooledByteBufAllocator.DEFAULT.buffer(
+                Math.min(Math.max(initialBufferSize * 6 / 5, HEADER_LENGTH), MAX_LOGRECORDSET_SIZE),


why this *6/5 ?

I don't remember what was exactly the reason that we used *6/5. We can probably remove it.

leighst · 2017-08-30T03:44:16Z

distributedlog-core/src/main/java/org/apache/distributedlog/EnvelopedEntryWriter.java

+
+        if (Type.NONE == codec) {
+            // update version
+            buffer.setByte(0, CURRENT_VERSION);


can you avoid these magic numbers and use constants or field size variables? or a relative setter? (set(dataLen))

added CONSTANTS for the offsets.

leighst · 2017-08-30T03:49:12Z

distributedlog-protocol/src/main/java/org/apache/distributedlog/EnvelopedRecordSetReader.java

    }

+    @Override
+    protected void finalize() throws Throwable {


+1 on this, seems like its not something we should do lightly

how are you planning to solve it?

leighst · 2017-08-30T03:51:26Z

distributedlog-protocol/src/main/java/org/apache/distributedlog/LogRecord.java

@@ -184,7 +192,23 @@ protected void setTransactionId(long txid) {
     * @return payload of this log record.
     */
    public byte[] getPayload() {


why is this still necessary? back compat?

I kept it there for back compat.

sijie · 2017-08-30T18:33:04Z

@leighst : I address your comments. also removed the #finalize() approach.

LogRecord: copy the memory for reads, because we don't know how long the record will be held by the application, there is no simple way to release the reference
All the other classes - managing releasing the reference count.

eolivelli

Great work @sijie

leighst

#shipit

sijie · 2017-08-31T06:38:44Z

FYI. the new compression codec shows much better result.

this patch:

Benchmark                                                                 (size)   Mode  Cnt      Score      Error   Units
CompressionBenchmark.testCompressLZ4                                          10  thrpt   50   6382.946 ±  252.338  ops/ms
CompressionBenchmark.testCompressLZ4                                         100  thrpt   50    560.667 ±   15.716  ops/ms
CompressionBenchmark.testCompressLZ4                                        1000  thrpt   50    198.630 ±    4.767  ops/ms
CompressionBenchmark.testCompressLZ4                                       10000  thrpt   50     61.814 ±    1.543  ops/ms
CompressionBenchmark.testCompressNone                                         10  thrpt   50  48918.275 ±  688.621  ops/ms
CompressionBenchmark.testCompressNone                                        100  thrpt   50  47937.500 ± 3246.851  ops/ms
CompressionBenchmark.testCompressNone                                       1000  thrpt   50  49029.124 ±  982.252  ops/ms
CompressionBenchmark.testCompressNone                                      10000  thrpt   50  50717.187 ± 1143.477  ops/ms
CompressionBenchmark.testDecompressLZ4                                        10  thrpt   50   7191.179 ±  196.163  ops/ms
CompressionBenchmark.testDecompressLZ4                                       100  thrpt   50   4435.242 ±   89.038  ops/ms
CompressionBenchmark.testDecompressLZ4                                      1000  thrpt   50    994.583 ±   22.731  ops/ms
CompressionBenchmark.testDecompressLZ4                                     10000  thrpt   50    112.235 ±    1.937  ops/ms
CompressionBenchmark.testDecompressNone                                       10  thrpt   50  36554.827 ±  571.760  ops/ms
CompressionBenchmark.testDecompressNone                                      100  thrpt   50  37407.131 ±  711.580  ops/ms
CompressionBenchmark.testDecompressNone                                     1000  thrpt   50  37218.008 ±  910.462  ops/ms
CompressionBenchmark.testDecompressNone                                    10000  thrpt   50  40611.669 ±  509.468  ops/ms

0.4.0-incubating

CompressionBenchmark.testCompressLZ4                                          10  thrpt   50    2422.408 ±   44.161  ops/ms
CompressionBenchmark.testCompressLZ4                                         100  thrpt   50    1793.394 ±   27.825  ops/ms
CompressionBenchmark.testCompressLZ4                                        1000  thrpt   50     928.596 ±   14.488  ops/ms
CompressionBenchmark.testCompressLZ4                                       10000  thrpt   50     239.443 ±    4.752  ops/ms
CompressionBenchmark.testCompressNone                                         10  thrpt   50  207570.657 ± 4656.234  ops/ms
CompressionBenchmark.testCompressNone                                        100  thrpt   50   90762.739 ± 1753.916  ops/ms
CompressionBenchmark.testCompressNone                                       1000  thrpt   50   10532.599 ±  207.739  ops/ms
CompressionBenchmark.testCompressNone                                      10000  thrpt   50    1034.792 ±   20.067  ops/ms
CompressionBenchmark.testDecompressLZ4                                        10  thrpt   50    4265.879 ±   80.825  ops/ms
CompressionBenchmark.testDecompressLZ4                                       100  thrpt   50    3975.214 ±  130.441  ops/ms
CompressionBenchmark.testDecompressLZ4                                      1000  thrpt   50    1821.820 ±   37.923  ops/ms
CompressionBenchmark.testDecompressLZ4                                     10000  thrpt   50     298.692 ±    8.591  ops/ms
CompressionBenchmark.testDecompressNone                                       10  thrpt   50  202194.272 ± 4431.166  ops/ms
CompressionBenchmark.testDecompressNone                                      100  thrpt   50   80907.829 ± 3869.202  ops/ms
CompressionBenchmark.testDecompressNone                                     1000  thrpt   50   10041.016 ±  391.215  ops/ms
CompressionBenchmark.testDecompressNone                                    10000  thrpt   50    1016.756 ±   26.645  ops/ms

sijie and others added 14 commits July 19, 2017 16:33

Bump BK version to 4.5.0-SNAPSHOT

2f25d9f

- registerSuccessEvent and registerFailureEvent needs TimeUnit - change HashedWheelTimer to netty 4 - change channelFactory to eventLoopGroup

DL-2: DistributedLog should work with the official apache bookkeeper …

ccd7448

…version

pass compile

6957e46

move twttr repo

a047cf6

bump curator to 4.0

67014df

exclude .repo

892ebe2

Merge remote-tracking branch 'apache/master' into bump_dl_version

d4fdfcf

reduce the number of threads used for tests

df71a83

ByteBuf in distributedlog-protocol

65b7c79

Zero copy in distributedlog-core

c49b290

Zero copy on distributedlog proxies

bcddeae

Clean up

d0ce73c

Merge remote-tracking branch 'apache/master' into zero_copy

5b697cf

Organize the rat

196f80b

sijie added area/core release/0.5.0 type/feature labels Aug 22, 2017

sijie added this to the 0.5.0 milestone Aug 22, 2017

sijie self-assigned this Aug 22, 2017

sijie requested a review from mgodave August 22, 2017 19:12

eolivelli reviewed Aug 23, 2017

View reviewed changes

hellostreaming approved these changes Aug 24, 2017

View reviewed changes

hellostreaming mentioned this pull request Aug 30, 2017

Issue 157: Introduce JMH for micro benchmarking #158

Closed

leighst approved these changes Aug 30, 2017

View reviewed changes

sijie added 3 commits August 29, 2017 21:53

Merge branch 'master' into zero_copy

b5e7e97

Remove finalize

1c67e78

Fix test

32a97c7

eolivelli approved these changes Aug 30, 2017

View reviewed changes

leighst approved these changes Aug 31, 2017

View reviewed changes

Fix NPE

4e747a6

use buffer

c9b3452

sijie added the type/task label Aug 31, 2017

sijie closed this in 46a3840 Aug 31, 2017

Conversation

sijie commented Aug 22, 2017

Uh oh!

sijie commented Aug 22, 2017

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hellostreaming left a comment

Choose a reason for hiding this comment

Uh oh!

sijie commented Aug 30, 2017

Uh oh!

leighst left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sijie commented Aug 30, 2017

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

leighst left a comment

Choose a reason for hiding this comment

Uh oh!

sijie commented Aug 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sijie commented Aug 31, 2017 •

edited

Loading