[GitHub] [kafka] ijuma commented on pull request #9229: MINOR: Reduce allocations in requests via buffer caching
ijuma commented on pull request #9229: URL: https://github.com/apache/kafka/pull/9229#issuecomment-851047311 The results are similar for the ducktape benchmarks since the bottleneck is elsewhere. In the PR description, I include the results for a workload that shows significant improvement with these changes. Also, the following allocation profiles show that the the lz4 buffer allocations dominate trunk and are gone in this PR: trunk:  this PR:  So, I think we can go ahead and merge this. -- 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: us...@infra.apache.org
[GitHub] [kafka] ijuma commented on pull request #9229: MINOR: Reduce allocations in requests via buffer caching
ijuma commented on pull request #9229:
URL: https://github.com/apache/kafka/pull/9229#issuecomment-850747419
This PR:
>
> SESSION REPORT (ALL TESTS)
> ducktape version: 0.8.1
> session_id: 2021-05-28--005
> run time: 16 minutes 56.024 seconds
> tests run:18
> passed: 18
> failed: 0
> ignored: 0
>
> test_id:
kafkatest.benchmarks.core.benchmark_test.Benchmark.test_consumer_throughput.security_protocol=PLAINTEXT.compression_type=lz4
> status: PASS
> run time: 1 minute 16.243 seconds
> {"records_per_sec": 2044571.6622, "mb_per_sec": 194.9855}
>
> test_id:
kafkatest.benchmarks.core.benchmark_test.Benchmark.test_consumer_throughput.security_protocol=PLAINTEXT.compression_type=zstd
> status: PASS
> run time: 1 minute 19.227 seconds
> {"records_per_sec": 1779992.88, "mb_per_sec": 169.7533}
>
> test_id:
kafkatest.benchmarks.core.benchmark_test.Benchmark.test_producer_and_consumer.security_protocol=PLAINTEXT.compression_type=lz4
> status: PASS
> run time: 1 minute 13.064 seconds
> {"producer": {"records_per_sec": 402868.423173, "mb_per_sec": 38.42},
"consumer": {"records_per_sec": 408363.28, "mb_per_sec": 38.9446}}
>
> test_id:
kafkatest.benchmarks.core.benchmark_test.Benchmark.test_producer_and_consumer.security_protocol=PLAINTEXT.compression_type=zstd
> status: PASS
> run time: 1 minute 12.112 seconds
> {"producer": {"records_per_sec": 347886.588972, "mb_per_sec": 33.18},
"consumer": {"records_per_sec": 352534.7247, "mb_per_sec": 33.6203}}
>
> test_id:
kafkatest.benchmarks.core.benchmark_test.Benchmark.test_end_to_end_latency.security_protocol=PLAINTEXT.compression_type=lz4
> status: PASS
> run time: 51.120 seconds
> {"latency_50th_ms": 0.0, "latency_99th_ms": 3.0, "latency_999th_ms": 8.0}
>
> test_id:
kafkatest.benchmarks.core.benchmark_test.Benchmark.test_end_to_end_latency.security_protocol=PLAINTEXT.compression_type=zstd
> status: PASS
> run time: 45.992 seconds
> {"latency_50th_ms": 0.0, "latency_99th_ms": 3.0, "latency_999th_ms": 9.0}
>
> test_id:
kafkatest.benchmarks.core.benchmark_test.Benchmark.test_long_term_producer_throughput.security_protocol=PLAINTEXT.compression_type=lz4
> status: PASS
> run time: 1 minute 11.957 seconds
> {"0": {"records_per_sec": 400994.466276, "mb_per_sec": 38.24}}
>
> test_id:
kafkatest.benchmarks.core.benchmark_test.Benchmark.test_long_term_producer_throughput.security_protocol=PLAINTEXT.compression_type=zstd
> status: PASS
> run time: 1 minute 12.859 seconds
> {"0": {"records_per_sec": 366716.784627, "mb_per_sec": 34.97}}
>
> test_id:
kafkatest.benchmarks.core.benchmark_test.Benchmark.test_producer_throughput.acks=1.topic=topic-replication-factor-three.security_protocol=PLAINTEXT.compression_type=lz4.message_size=10
> status: PASS
> run time: 55.828 seconds
> {"records_per_sec": 1101318.782309, "mb_per_sec": 10.5}
>
> test_id:
kafkatest.benchmarks.core.benchmark_test.Benchmark.test_producer_throughput.acks=1.topic=topic-replication-factor-three.security_protocol=PLAINTEXT.compression_type=lz4.message_size=100
> status: PASS
> run time: 44.917 seconds
> {"records_per_sec": 373345.479833, "mb_per_sec": 35.6}
>
> test_id:
kafkatest.benchmarks.core.benchmark_test.Benchmark.test_producer_throughput.acks=1.topic=topic-replication-factor-three.security_protocol=PLAINTEXT.compression_type=lz4.message_size=1000
> status: PASS
> run time: 45.609 seconds
> {"records_per_sec": 63912.857143, "mb_per_sec": 60.95}
>
> test_id:
[GitHub] [kafka] ijuma commented on pull request #9229: MINOR: Reduce allocations in requests via buffer caching
ijuma commented on pull request #9229: URL: https://github.com/apache/kafka/pull/9229#issuecomment-814999556 @chia7712 When you say performance test, you mean the ducktape ones? -- 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: us...@infra.apache.org
[GitHub] [kafka] ijuma commented on pull request #9229: MINOR: Reduce allocations in requests via buffer caching
ijuma commented on pull request #9229: URL: https://github.com/apache/kafka/pull/9229#issuecomment-814141618 @chia7712 I updated the PR. Please review when you have a chance. -- 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: us...@infra.apache.org
[GitHub] [kafka] ijuma commented on pull request #9229: MINOR: Reduce allocations in requests via buffer caching
ijuma commented on pull request #9229: URL: https://github.com/apache/kafka/pull/9229#issuecomment-814090052 @chia7712 Yes, it's worth exploring. I think `MemoryPool` is intended to be a thread-safe cache, so it's not trivial, but it may be possible. Are you interested in looking into that? -- 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: us...@infra.apache.org
[GitHub] [kafka] ijuma commented on pull request #9229: MINOR: Reduce allocations in requests via buffer caching
ijuma commented on pull request #9229: URL: https://github.com/apache/kafka/pull/9229#issuecomment-813439278 @chia7712 I introduced `RequestLocal` as discussed. Does this seem reasonable to you? If so, I propose the following next steps: 1. In this PR, provide utility methods in `RequestLocal` for the two common defaults: `ThreadLocalCaching` and `NoCaching`. The latter should be used when the usage is not guaranteed to be within the same thread. In the future, we can consider a `ThreadSafeCaching`/`GlobalCaching` option, if that makes sense. 2. In a separate PR, remove the default arguments. This will result in a lot of test changes, but no change in behavior. So, it probably makes sense to review separately. Thoughts? -- 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: us...@infra.apache.org
[GitHub] [kafka] ijuma commented on pull request #9229: MINOR: Reduce allocations in requests via buffer caching
ijuma commented on pull request #9229: URL: https://github.com/apache/kafka/pull/9229#issuecomment-778804930 We already have a `RequestContext` class, so another name would be `RequestLocal` (similar to `ThreadLocal`). Thoughts @chia7712? 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: us...@infra.apache.org
[GitHub] [kafka] ijuma commented on pull request #9229: MINOR: Reduce allocations in requests via buffer caching
ijuma commented on pull request #9229: URL: https://github.com/apache/kafka/pull/9229#issuecomment-767687131 @chia7712 I had forgotten that part of the discussion. :) Let me take a closer look at that. 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: us...@infra.apache.org
[GitHub] [kafka] ijuma commented on pull request #9229: MINOR: Reduce allocations in requests via buffer caching
ijuma commented on pull request #9229: URL: https://github.com/apache/kafka/pull/9229#issuecomment-767662119 @chia7712 One question we have to decide is whether we want to remove the default arguments in this PR or in a separate PR that is purely mechanical (no behavior changes). A lot of tests call the relevant methods, so removing the defaults would cause a lot of test changes. I am leaning towards doing it as a separate PR and maybe after the 2.8 branch is cut (to avoid disrupting other work targeting 2.8). What do you think? 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: us...@infra.apache.org
[GitHub] [kafka] ijuma commented on pull request #9229: MINOR: Reduce allocations in requests via buffer caching
ijuma commented on pull request #9229: URL: https://github.com/apache/kafka/pull/9229#issuecomment-683352558 @chia7712 One option would be for me to introduce a `RequestContext` case class and add the `BufferSupplier` as one of the fields. It would be easy to extend this class with request bound elements like `ActionQueue`. Thoughts? 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: us...@infra.apache.org
[GitHub] [kafka] ijuma commented on pull request #9229: MINOR: Reduce allocations in requests via buffer caching
ijuma commented on pull request #9229: URL: https://github.com/apache/kafka/pull/9229#issuecomment-683352094 In my opinion, thread locals are most useful when one doesn't control the code. For cases like this, being explicit makes it easier to reason about and also test. Even if it's a bit more work initially. 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: us...@infra.apache.org
