[improve][client] Add schema cache to improve performance #23808

yunmaoQu · 2025-01-03T16:46:14Z

Motivation

Schema creation (e.g., Schema.AVRO(SomeClass.class)) is fairly CPU intensive. It would be useful it there would be a weak reference cache for caching the schema instance.

Modifications

Add SchemaCache implementation using WeakHashMap for schema instance caching

Documentation

doc
doc-required
doc-not-needed
doc-complete

Matching PR in forked repository

N/A

github-actions · 2025-01-03T16:46:44Z

@yunmaoQu Please add the following content to your PR description and select a checkbox:

- [ ] `doc` <!-- Your PR contains doc changes -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [ ] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->

lhotari

There are several inconsistencies in this PR. For example, the class names and the class file names don't match. Please test this PR in your own fork first to ensure that it passes tests.
It seems that this PR contains a lot of features related to the schema caching. Instead of adding a lot of features, it would be better to keep the implementation to the minimum.
I'm surprised by competing implementations for implementing the schema cache. There's currently already an open PR #23777.

yunmaoQu · 2025-01-03T18:38:06Z

ok,i test all and could you review it and give me some suggestions

lhotari · 2025-01-03T19:06:48Z

ok,i test all and could you review it and give me some suggestions

Instead of adding more code to test everything, please reduce to a minimal implementation. This means to remove features to track cache metrics. That's not something that is needed. For the cache implementation, I'd suggest using a ConcurrentMap created with Guava's MapMaker. Instead of adding yet another abstraction, I'd suggest modifying the PulsarClientImplementationBinding interface and adding a new interface method <T extends com.google.protobuf.GeneratedMessageV3> Schema<T> newProtobufSchema(Class<T> clazz). Then we could keep the cache as an implementation level detail.

example of minimal implementation for newProtobufSchema using Guava's MapMaker with weak keys:

    private static final ConcurrentMap<Class<?>, Schema<?>> PROTOBUF_CACHE = new MapMaker().weakKeys().makeMap();

    public <T extends com.google.protobuf.GeneratedMessageV3> Schema<T> newProtobufSchema(Class<T> clazz) {
        return (Schema<T>) PROTOBUF_CACHE.computeIfAbsent(clazz,
                k -> ProtobufSchema.of(SchemaDefinition.builder().withPojo(clazz).build())).clone();
    }

There shouldn't be a need to ever clear the cache since it's bounded by the number of classes with strong references. It won't consume a significant amount of memory in the first place.

yunmaoQu · 2025-01-03T19:14:15Z

OK.Should i implement it based on the pre commit or what?

lhotari · 2025-01-03T19:26:36Z

OK.Should i implement it based on the pre commit or what?

That's something you can decide. Please read my previous message and draw your conclusions.

walkinggo · 2025-01-04T04:07:11Z

It looks like we're working on similar tasks. I've already created a pull request #23777 to complete this task. Should we work together to finish it, or what do you suggest? @yunmaoQu

yunmaoQu · 2025-01-04T04:57:01Z

Yes. We can work it together.@walkinggo

yunmaoQu · 2025-01-05T18:06:40Z

@lhotari

OK.Should i implement it based on the pre commit or what?

That's something you can decide. Please read my previous message and draw your conclusions.

I implement a minimal version. Could you review it and give me some suggestion. Thanks for your previous guide.

lhotari

This is very close to the minimal implementation. I added a few minor comments.

...lient/src/main/java/org/apache/pulsar/client/impl/PulsarClientImplementationBindingImpl.java

lhotari · 2025-01-06T18:52:15Z

Please also update the PR description to match the minimal implementation.

yunmaoQu · 2025-01-07T04:45:25Z

Please also update the PR description to match the minimal implementation.

@lhotari I have done this.

lhotari · 2025-01-07T09:12:10Z

Please also update the PR description to match the minimal implementation.

@lhotari I have done this.

@yunmaoQu I don't see that the PR description has been updated to match the minimal implementation. For example, the "modifications" part hasn't been updated.

Add SchemaCache implementation using WeakHashMap for schema instance caching

Add cache configuration and metrics for monitoring

Add cleanup strategy for expired cache entries

Modify Schema creation methods (AVRO/JSON/PROTOBUF) to use cache

Add cloning mechanism to maintain schema immutability

lhotari · 2025-01-07T09:15:22Z

checkstyle error:

[INFO] There is 1 error reported by Checkstyle 10.14.2 with /home/runner/work/pulsar/pulsar/buildtools/src/main/resources/pulsar/checkstyle.xml ruleset.
Error: src/main/java/org/apache/pulsar/client/internal/PulsarClientImplementationBinding.java:[130] (regexp) RegexpSingleline: Trailing whitespace

lhotari · 2025-01-07T09:18:08Z

@yunmaoQu I'd recommend to run CI builds in your fork so that you get CI feedback while working on the changes. Some of that is explained in the contribution guide, https://pulsar.apache.org/contribute/personal-ci/ . You will also need to enable GitHub Actions in your apache/pulsar fork repository in GitHub UI.

yunmaoQu · 2025-01-07T10:47:07Z

@yunmaoQu I'd recommend to run CI builds in your fork so that you get CI feedback while working on the changes. Some of that is explained in the contribution guide, https://pulsar.apache.org/contribute/personal-ci/ . You will also need to enable GitHub Actions in your apache/pulsar fork repository in GitHub UI.

Thanks a lot.

- add a weak reference cache for caching a scheme instance for Schema.AVRO, Schema.JSON, Schema.PROTOBUF.

…re/schema-cache

yunmaoQu · 2025-01-07T14:48:30Z

@lhotari I've modified the error according to the CI's prompt. But when i run personal CI ,it still reports an error.

lhotari · 2025-01-07T14:57:15Z

@lhotari I've modified the error according to the CI's prompt, but it still reports an error.

CI isn't the only choice. You can also reproduce the errors locally.

For CI feedback, I'd recommend creating a PR in your own fork so that the PR appears at https://github.com/yunmaoQu/pulsar/pulls . When you push changes to the branch, the CI will trigger and you won't have to depend on CI feedback from apache/pulsar CI runs which will only run after someone approves the workflow run.

lhotari · 2025-01-07T15:02:00Z

For locally running a sanity check, you can use this command:

mvn -Pcore-modules,-main -T 1C clean install -DskipTests -Dspotbugs.skip=true -DnarPluginPhase=none

lhotari · 2025-01-07T15:03:25Z

There are multiple checkstyle errors:

[ERROR] src/main/java/org/apache/pulsar/client/impl/PulsarClientImplementationBindingImpl.java:[33,1] (imports) ImportOrder: Import java.util.concurrent.ConcurrentMap appears after other imports that it should precede
[ERROR] src/main/java/org/apache/pulsar/client/impl/PulsarClientImplementationBindingImpl.java:[88,1] (imports) ImportOrder: Import com.google.common.collect.MapMaker appears after other imports that it should precede
[ERROR] src/main/java/org/apache/pulsar/client/impl/PulsarClientImplementationBindingImpl.java:[222] (regexp) RegexpSingleline: Trailing whitespace
[ERROR] src/main/java/org/apache/pulsar/client/impl/PulsarClientImplementationBindingImpl.java:[242] (regexp) RegexpSingleline: Trailing whitespace
[ERROR] src/main/java/org/apache/pulsar/client/impl/PulsarClientImplementationBindingImpl.java:[251] (regexp) RegexpSingleline: Trailing whitespace

I'd recommend following the contribution guide to properly configure IntelliJ/IDEA for Pulsar development.

lhotari

The unnecessary cleanup strategy stuff is back. We don't need those type of features.

yunmaoQu · 2025-01-07T17:07:41Z

The unnecessary cleanup strategy stuff is back. We don't need those type of features.

Very Sorry. A little git operation error.

yunmaoQu · 2025-01-08T04:00:49Z

@lhotari I test CI in my personal repo，the part i change is ok, you can see https://github.com/yunmaoQu/pulsar/pulls

lhotari

The changes are now fine for the minimal implementation. I did some manual checks and noticed that adding this cache won't resolve the performance issue. For Avro schema, the problem is that the .clone() method doesn't by-pass the already parsed Avro schema instance to the new cloned instance. Since it might be hard to detect such issues, it would be necessary to have a micro benchmark which could be used to detect the issues. In Pulsar, we have the module microbench where such a benchmark could be added. The module has JMH configured. In JMH, it's also possible to enable profiling using AsyncProfiler or Java Flight Recorder to find the performance hotspots. I won't be able to guide through all steps required to handle this. It would be necessary to add a micro benchmark and then address the performance issues that show up.

yunmaoQu · 2025-01-08T09:38:36Z

@lhotari This pr seems make no sense .Should i close it and focus on other issue?

lhotari · 2025-01-08T09:47:50Z

@lhotari This pr seems make no sense .Should i close it and focus on other issue?

@yunmaoQu You can choose to do that. I'm not your boss. :) In many cases, finding a solution isn't a direct path. That's also the case here.

github-actions bot added the doc-label-missing label Jan 3, 2025

github-actions bot added doc-not-needed Your PR changes do not impact docs and removed doc-label-missing labels Jan 3, 2025

lhotari requested changes Jan 3, 2025

View reviewed changes

lhotari mentioned this pull request Jan 3, 2025

[Enhancement] Cache Schema instances for classes in a weak reference cache since creating an instance could be CPU intensive #23777

Open

14 tasks

lhotari requested changes Jan 6, 2025

View reviewed changes

lhotari added this to the 4.1.0 milestone Jan 6, 2025

lhotari added the release/4.0.3 label Jan 6, 2025

lhotari added the ready-to-test label Jan 7, 2025

[Enhancement] Add schema cache to improve performance

fd24bd1

- add a weak reference cache for caching a scheme instance for Schema.AVRO, Schema.JSON, Schema.PROTOBUF.

yunmaoQu force-pushed the feature/schema-cache branch from 941beb6 to fd24bd1 Compare January 7, 2025 11:06

yunmaoQu and others added 3 commits January 7, 2025 21:34

Merge branch 'apache:master' into feature/schema-cache

59a1f81

fix CI error

232073c

Merge remote-tracking branch 'origin/feature/schema-cache' into featu…

1cf7b61

…re/schema-cache

lhotari requested changes Jan 7, 2025

View reviewed changes

yunmaoQu force-pushed the feature/schema-cache branch from 9bfbe8a to 1cf7b61 Compare January 7, 2025 17:19

yunmaoQu added 2 commits January 7, 2025 17:32

fix CI error

bc32f38

fix CI error

f3c894e

lhotari reviewed Jan 8, 2025

View reviewed changes

yunmaoQu closed this Jan 8, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[improve][client] Add schema cache to improve performance #23808

[improve][client] Add schema cache to improve performance #23808

yunmaoQu commented Jan 3, 2025 •

edited

Loading

github-actions bot commented Jan 3, 2025

lhotari left a comment

yunmaoQu commented Jan 3, 2025

lhotari commented Jan 3, 2025

yunmaoQu commented Jan 3, 2025

lhotari commented Jan 3, 2025

walkinggo commented Jan 4, 2025 •

edited

Loading

yunmaoQu commented Jan 4, 2025 •

edited

Loading

yunmaoQu commented Jan 5, 2025 •

edited

Loading

lhotari left a comment

lhotari commented Jan 6, 2025

yunmaoQu commented Jan 7, 2025 •

edited

Loading

lhotari commented Jan 7, 2025

lhotari commented Jan 7, 2025

lhotari commented Jan 7, 2025

yunmaoQu commented Jan 7, 2025

yunmaoQu commented Jan 7, 2025 •

edited

Loading

lhotari commented Jan 7, 2025

lhotari commented Jan 7, 2025

lhotari commented Jan 7, 2025

lhotari left a comment

yunmaoQu commented Jan 7, 2025 •

edited

Loading

yunmaoQu commented Jan 8, 2025

lhotari left a comment

yunmaoQu commented Jan 8, 2025 •

edited

Loading

lhotari commented Jan 8, 2025

[improve][client] Add schema cache to improve performance #23808

[improve][client] Add schema cache to improve performance #23808

Conversation

yunmaoQu commented Jan 3, 2025 • edited Loading

Motivation

Modifications

Documentation

Matching PR in forked repository

github-actions bot commented Jan 3, 2025

lhotari left a comment

Choose a reason for hiding this comment

yunmaoQu commented Jan 3, 2025

lhotari commented Jan 3, 2025

yunmaoQu commented Jan 3, 2025

lhotari commented Jan 3, 2025

walkinggo commented Jan 4, 2025 • edited Loading

yunmaoQu commented Jan 4, 2025 • edited Loading

yunmaoQu commented Jan 5, 2025 • edited Loading

lhotari left a comment

Choose a reason for hiding this comment

lhotari commented Jan 6, 2025

yunmaoQu commented Jan 7, 2025 • edited Loading

lhotari commented Jan 7, 2025

lhotari commented Jan 7, 2025

lhotari commented Jan 7, 2025

yunmaoQu commented Jan 7, 2025

yunmaoQu commented Jan 7, 2025 • edited Loading

lhotari commented Jan 7, 2025

lhotari commented Jan 7, 2025

lhotari commented Jan 7, 2025

lhotari left a comment

Choose a reason for hiding this comment

yunmaoQu commented Jan 7, 2025 • edited Loading

yunmaoQu commented Jan 8, 2025

lhotari left a comment

Choose a reason for hiding this comment

yunmaoQu commented Jan 8, 2025 • edited Loading

lhotari commented Jan 8, 2025

yunmaoQu commented Jan 3, 2025 •

edited

Loading

walkinggo commented Jan 4, 2025 •

edited

Loading

yunmaoQu commented Jan 4, 2025 •

edited

Loading

yunmaoQu commented Jan 5, 2025 •

edited

Loading

yunmaoQu commented Jan 7, 2025 •

edited

Loading

yunmaoQu commented Jan 7, 2025 •

edited

Loading

yunmaoQu commented Jan 7, 2025 •

edited

Loading

yunmaoQu commented Jan 8, 2025 •

edited

Loading