Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[improve][java-client]Add init capacity for messages in BatchMessageContainerImpl #17822

Merged
merged 5 commits into from
Sep 30, 2022

Conversation

AnonHxy
Copy link
Contributor

@AnonHxy AnonHxy commented Sep 23, 2022

Motivation

When adding messages to BatchMessageContainerImpl, it takes a lot of cpu time for the list growing of BatchMessageContainerImpl#messages, see below:
image

Modifications

  • Init BatchMessageContainerImpl#messages with capacity.

Verifying this change

  • Make sure that the change passes the CI checks.

Documentation

  • doc-required
    (Your PR needs to update docs and you will update later)

  • doc-not-needed
    (Please explain why)

  • doc
    (Your PR contains doc changes)

  • doc-complete
    (Docs have been already added)

Matching PR in forked repository

PR in forked repository: AnonHxy#3

@AnonHxy AnonHxy self-assigned this Sep 23, 2022
@AnonHxy AnonHxy requested a review from merlimat September 23, 2022 11:29
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Sep 23, 2022
@AnonHxy AnonHxy force-pushed the opt_producer_array_grow branch from f3b59bf to 1d7f302 Compare September 25, 2022 05:32
@AnonHxy AnonHxy added this to the 2.12.0 milestone Sep 26, 2022
Copy link
Contributor

@Jason918 Jason918 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@eolivelli eolivelli changed the title [improve][java-client]Add init capactiy for messages in BatchMessageContainerImpl [improve][java-client]Add init capacity for messages in BatchMessageContainerImpl Sep 26, 2022
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

it is worth to cherry-pick this to branch-2.10 and branch-2.11

@AnonHxy
Copy link
Contributor Author

AnonHxy commented Sep 27, 2022

/pulsarbot ready-to-test

@AnonHxy AnonHxy closed this Sep 27, 2022
@AnonHxy AnonHxy reopened this Sep 27, 2022
@lhotari
Copy link
Member

lhotari commented Sep 27, 2022

@AnonHxy just curious which VM/platform you are running on.

I'm wondering about the large portion of "gettimeofday" as part of the stack
image

Regarding "gettimeofday", there's a Brendan Gregg blog post "The Speed Of Time". In many cases, the fastest clocksource on Linux is tsc. GCP's GKE uses tsc by default.

Since Arrays.copyOf / System.arraycopy (or it's native implementations) don't seem to call gettimeofday at all, it might be GC related activity which is happening.

The default initial size for ArrayList is 10. In this PR it's changed to 64. That could make sense. However, I would assume that the average batch size is very rarely over 20, so this change won't make a difference in most cases. Since average batch size is rarely over 20, a better default could be a lower value, such as 32.

@lhotari
Copy link
Member

lhotari commented Sep 27, 2022

the gettimeofday stack looks like MacOS, https://opensource.apple.com/source/xnu/xnu-3789.41.3/libsyscall/wrappers/__commpage_gettimeofday.c.auto.html
Profiling results can be flawed on MacOS.

@AnonHxy
Copy link
Contributor Author

AnonHxy commented Sep 27, 2022

Profiling results can be flawed on MacOS.

Thanks for your reminder @lhotari , There is anothoer profiling result on Linux, the simiar conclusions can be drawn.
image

@AnonHxy
Copy link
Contributor Author

AnonHxy commented Sep 27, 2022

Since average batch size is rarely over 20, a better default could be a lower value, such as 32.

It makes sense to me. Updated default capacity from 64 to 32. PTAL @lhotari

@lhotari
Copy link
Member

lhotari commented Sep 27, 2022

Profiling results can be flawed on MacOS.

Thanks for your reminder @lhotari , There is anothoer profiling result on Linux, the simiar conclusions can be drawn. image

Can you share the whole flamegraph? A lot of information is lost if you just provide a snippet of the whole graph.

@AnonHxy
Copy link
Contributor Author

AnonHxy commented Sep 27, 2022

Can you share the whole flamegraph?

OK, here is the flamegraph file. @lhotari
buffer.html.zip

test case is below

public static void main(String[] args) throws Exception {
        String topic = "hxy-test-mem";
        byte[] smallMsg = new byte[64];
        PulsarClient client = PulsarClient.builder()
                .serviceUrl("pulsar://127.0.0.1:6650")
                .memoryLimit(2000, SizeUnit.MEGA_BYTES)  // memory limit 2000M
                .build();
        Producer<byte[]> producer = client.newProducer()
                .topic(topic)
                .enableBatching(true)
                .create();
        for (int i = 0; i < Integer.MAX_VALUE; ++i) {
            producer.sendAsync(smallMsg);
        }
    }

@AnonHxy
Copy link
Contributor Author

AnonHxy commented Sep 29, 2022

/pulsarbot run-failure-checks

@AnonHxy AnonHxy merged commit 15a347c into apache:master Sep 30, 2022
liangyepianzhou pushed a commit that referenced this pull request Dec 14, 2022
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Jan 10, 2023
…ontainerImpl (apache#17822)

(cherry picked from commit 15a347c)
(cherry picked from commit 166a7ab)
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Jan 11, 2023
…ontainerImpl (apache#17822)

(cherry picked from commit 15a347c)
(cherry picked from commit 166a7ab)
Technoboy- pushed a commit that referenced this pull request Feb 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants