CAMEL-19815: use a ConcurrentHashMap instead of a blanket synchronize by orpiske · Pull Request #11279 · apache/camel

orpiske · 2023-09-01T14:29:55Z

This should help offset the performance impacts of removing the cached QueueReference on d66469d

4.1 (this patch)

Benchmark                                         Mode  Cnt    Score    Error   Units
SedaRoundTripTest.sendBlocking                   thrpt   10  925.275 ±  1.146  ops/ms
SedaRoundTripTest.sendBlockingWithMultipleTypes  thrpt   10  229.778 ±  0.107  ops/ms
SedaRoundTripTest.sendBlockingWithMultipleTypes   avgt   10    0.004 ±  0.001   ms/op
SedaRoundTripTest.sendBlocking                      ss   10    0.163 ±  0.035   ms/op
SedaRoundTripTest.sendBlockingWithMultipleTypes     ss   10    0.570 ±  0.047   ms/op

4.1 (without the patch)

Benchmark                                         Mode  Cnt    Score    Error   Units
SedaRoundTripTest.sendBlocking                   thrpt   10  915.329 ±  0.320  ops/ms
SedaRoundTripTest.sendBlockingWithMultipleTypes  thrpt   10  225.853 ±  0.459  ops/ms
SedaRoundTripTest.sendBlockingWithMultipleTypes   avgt   10    0.005 ±  0.001   ms/op
SedaRoundTripTest.sendBlocking                      ss   10    0.167 ±  0.025   ms/op
SedaRoundTripTest.sendBlockingWithMultipleTypes     ss   10    0.626 ±  0.222   ms/op

4.0 (baseline)

Benchmark                                         Mode  Cnt    Score    Error   Units
SedaRoundTripTest.sendBlocking                   thrpt   10  922.456 ±  0.462  ops/ms
SedaRoundTripTest.sendBlockingWithMultipleTypes  thrpt   10  242.381 ±  0.107  ops/ms
SedaRoundTripTest.sendBlockingWithMultipleTypes   avgt   10    0.004 ±  0.001   ms/op
SedaRoundTripTest.sendBlocking                      ss   10    0.162 ±  0.022   ms/op
SedaRoundTripTest.sendBlockingWithMultipleTypes     ss   10    0.573 ±  0.044   ms/op

github-actions · 2023-09-01T14:30:08Z

🌟 Thank you for your contribution to the Apache Camel project! 🌟

🐫 Maintainers, please note that first-time contributors require manual approval for the GitHub Actions to run.

⚠️ Please note that the changes on this PR may be tested automatically if they change components.

🤖 Use the command /component-test (camel-)component-name1 (camel-)component-name2.. to request a test from the test bot.

If necessary Apache Camel Committers may access logs and test results in the job summaries!

essobedo · 2023-09-01T14:42:09Z

components/camel-seda/src/main/java/org/apache/camel/component/seda/SedaComponent.java

-        ref = new QueueReference(queue, size, multipleConsumers);
+        QueueReference ref = new QueueReference(queue, size, multipleConsumers);
        ref.addReference(endpoint);
        getQueues().put(key, ref);


Maybe we should consider creating the queue and the queue reference in a computeIfAbsent to prevent concurrent creation for the same key

essobedo · 2023-09-01T14:45:56Z

components/camel-seda/src/main/java/org/apache/camel/component/seda/SedaEndpoint.java

-                if (ref.getSize() != null) {
-                    setSize(ref.getSize());
+            synchronized (this) {
+                if (queue == null) {


Warning, queue is not volatile so this is not thread-safe

essobedo · 2023-09-01T14:49:51Z

Maybe I misunderstood your results, but I don't see a real significant improvement, could you please clarify a bit your results?

orpiske · 2023-09-01T14:54:29Z

Marking it as a draft as I want to go through @essobedo's comments next week.

essobedo · 2023-09-01T14:57:13Z

Another question, Is there a real-life use case where we heavily create queues? otherwise, I don't really get the real need of improving it.

Do you have profiler reports indicating that this part of the code as an hot spot?

orpiske · 2023-09-01T15:00:00Z

Maybe I misunderstood your results, but I don't see a real significant improvement, could you please clarify a bit your results?

The key ones are the top two on each table. They are measured in terms of number of operations per millisecond. Even though the number seems small, when spread over the course of a few minutes/hours, this results in quite a lot more exchanges going through the core.

essobedo · 2023-09-01T15:06:13Z

You mean that we need to compare 925 ops/ms for 4.1 with 922 ops/ms for 4.0 in case of SedaRoundTripTest.sendBlocking and 229 ops/ms for 4.1 with 242 ops/ms for 4.0 in case of SedaRoundTripTest.sendBlockingWithMultipleTypes, right?

rhuan080 · 2023-09-01T15:06:39Z

components/camel-seda/src/main/java/org/apache/camel/component/seda/SedaComponent.java

+        }
        return ref;
    }



Can the 'synchronized' keyword in the 'registerQueue' method also be eliminated? I believe you can utilize 'computeIfAbsent,' as suggested by @essobedo in the comment above.

orpiske · 2023-09-01T15:07:28Z

Another question, Is there a real-life use case where we heavily create queues? otherwise, I don't really get the real need of improving it.

The getQueueReference is in the hot path for the SedaProducer addToQueue`.

Do you have profiler reports indicating that this part of the code as an hot spot?

Absolutely.

orpiske · 2023-09-01T15:12:29Z

You mean that we need to compare 925 ops/ms for 4.1 with 922 ops/ms for 4.0 in case of SedaRoundTripTest.sendBlocking and 229 ops/ms for 4.1 with 242 ops/ms for 4.0 in case of SedaRoundTripTest.sendBlockingWithMultipleTypes, right?

Exactly. This tries to bring the code closer to what we had in 4.0, as I had to revert one of the performance improvements we had there.

BTW, answering your other question: "Is there a real-life use case where we heavily create queues".

The answer is yes, we do. Basically, that patch for 4.0 was breaking a scenario for an user that was heavily creating queues.

essobedo · 2023-09-01T15:14:00Z

Interesting, by chance, do you have the html version of the reports to allow me to zoom in?

orpiske · 2023-09-01T15:14:52Z

Obs.: please, do feel free to review. I'll go through your review comments next week. For now just answering the non-review questions as my brain is basically a jelly after this week.

orpiske · 2023-09-01T15:24:07Z

Interesting, by chance, do you have the html version of the reports to allow me to zoom in?

Here's one: http://angusyoung.org/arquivos/tmp/profile-camel-19815-with-perf-v6-4.1.0-SNAPSHOT-2023-09-01-043702.html

Obs.: just keep in mind that this is just one of the many scenarios I run (and NOT generated from the same JMH results I pasted - it's just the only one I have readily available). If you want a different one (i.e.: uncontended SEDA or etc), drop me a note next week and I can send it to you (or point to how you can generate your own).

essobedo · 2023-09-01T15:35:31Z

You mean that we need to compare 925 ops/ms for 4.1 with 922 ops/ms for 4.0 in case of SedaRoundTripTest.sendBlocking and 229 ops/ms for 4.1 with 242 ops/ms for 4.0 in case of SedaRoundTripTest.sendBlockingWithMultipleTypes, right?

Exactly. This tries to bring the code closer to what we had in 4.0, as I had to revert one of the performance improvements we had there.

BTW, answering your other question: "Is there a real-life use case where we heavily create queues".

The answer is yes, we do. Basically, that patch for 4.0 was breaking a scenario for an user that was heavily creating queues.

If so if you check well the second case is better with Camel 4.0 and the first one is slightly better with Camel 4.1 but you have a higher Error 1.146 vs 0.462 which means that the results are more stable in Camel 4.0.

I'm wondering if an improvement of less than 1 % is insignificant enough to be considered a real improvement as the results may vary a lot from one test to another.

BTW, I've just realized that you set forks to 1, maybe you should retry with 3 to get more reliable results?

orpiske · 2023-09-01T15:43:42Z

You mean that we need to compare 925 ops/ms for 4.1 with 922 ops/ms for 4.0 in case of SedaRoundTripTest.sendBlocking and 229 ops/ms for 4.1 with 242 ops/ms for 4.0 in case of SedaRoundTripTest.sendBlockingWithMultipleTypes, right?

Exactly. This tries to bring the code closer to what we had in 4.0, as I had to revert one of the performance improvements we had there.
BTW, answering your other question: "Is there a real-life use case where we heavily create queues".
The answer is yes, we do. Basically, that patch for 4.0 was breaking a scenario for an user that was heavily creating queues.

If so if you check well the second case is better with Camel 4.0 and the first one is slightly better with Camel 4.1 but you have a higher Error 1.146 vs 0.462 which means that the results are more stable in Camel 4.0.

I'm wondering if an improvement of less than 1 % is insignificant enough to be considered a real improvement as the results may vary a lot from one test to another.

BTW, I've just realized that you set forks to 1, maybe you should retry with 3 to get more reliable results?

Normally that would be the case (i.e.: if we were comparing the results from the same test class). However, these tests are already running on forked executions: the iterations for different versions run separately (as in, the automation runs the tests for one version and then for another - not on the same JVM, I mean).

essobedo · 2023-09-01T15:46:02Z

Interesting, by chance, do you have the html version of the reports to allow me to zoom in?

Here's one: http://angusyoung.org/arquivos/tmp/profile-camel-19815-with-perf-v6-4.1.0-SNAPSHOT-2023-09-01-043702.html

Obs.: just keep in mind that this is just one of the many scenarios I run (and NOT generated from the same JMH results I pasted - it's just the only one I have readily available). If you want a different one (i.e.: uncontended SEDA or etc), drop me a note next week and I can send it to you (or point to how you can generate your own).

Thx for sharing do you have the same kind of report for 4.0 (the baseline)?

orpiske · 2023-09-01T15:48:29Z

Interesting, by chance, do you have the html version of the reports to allow me to zoom in?

Here's one: http://angusyoung.org/arquivos/tmp/profile-camel-19815-with-perf-v6-4.1.0-SNAPSHOT-2023-09-01-043702.html
Obs.: just keep in mind that this is just one of the many scenarios I run (and NOT generated from the same JMH results I pasted - it's just the only one I have readily available). If you want a different one (i.e.: uncontended SEDA or etc), drop me a note next week and I can send it to you (or point to how you can generate your own).

Thx for sharing do you have the same kind of report for 4.0 (the baseline)?

Not right now, as I am off from work, but on Monday I'll take a look at the review comments you raised (along w/ the ones from @rhuan080), adjust the code and regenerate all the data. I'll post the update report so you guys can look at it.

This should help offset the performance impacts of removing the cached QueueReference on d66469d

…/unboxing WIP: removing auto-boxing/unboxing

orpiske · 2023-09-05T05:43:13Z

Let's put this one on pause for now. I tried with the suggestions, but the performance wasn't better:

4.1.0-SNAPSHOT Updated

SedaRoundTripTest.sendBlocking                   thrpt   10  914.608 ±  0.545  ops/ms
SedaRoundTripTest.sendBlockingWithMultipleTypes  thrpt   10  232.482 ±  0.419  ops/ms

I'll get back to this one with a clearer head in a couple of weeks, after I dispatch other higher priority items.

Edit: tracking this on CAMEL-19830.

github-actions bot added components camel-4 labels Sep 1, 2023

essobedo reviewed Sep 1, 2023

View reviewed changes

orpiske marked this pull request as draft September 1, 2023 14:54

rhuan080 reviewed Sep 1, 2023

View reviewed changes

orpiske added 2 commits September 4, 2023 16:36

CAMEL-19815: use a ConcurrentHashMap instead of a blanket synchronize

4be82c3

This should help offset the performance impacts of removing the cached QueueReference on d66469d

(chores): cleanup usages of Integer to remove unnecessary auto-boxing…

26b436f

…/unboxing WIP: removing auto-boxing/unboxing

orpiske force-pushed the use-concurrent-hm branch from d4c0bc6 to 26b436f Compare September 4, 2023 14:37

orpiske closed this Sep 5, 2023

orpiske deleted the use-concurrent-hm branch April 10, 2024 14:46

Comments

Conversation

orpiske commented Sep 1, 2023

Uh oh!

github-actions bot commented Sep 1, 2023

Uh oh!

essobedo Sep 1, 2023

Choose a reason for hiding this comment

Uh oh!

essobedo Sep 1, 2023

Choose a reason for hiding this comment

Uh oh!

essobedo commented Sep 1, 2023

Uh oh!

orpiske commented Sep 1, 2023

Uh oh!

essobedo commented Sep 1, 2023

Uh oh!

orpiske commented Sep 1, 2023

Uh oh!

essobedo commented Sep 1, 2023

Uh oh!

rhuan080 Sep 1, 2023

Choose a reason for hiding this comment

Uh oh!

orpiske commented Sep 1, 2023

Uh oh!

orpiske commented Sep 1, 2023

Uh oh!

essobedo commented Sep 1, 2023

Uh oh!

orpiske commented Sep 1, 2023

Uh oh!

orpiske commented Sep 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

essobedo commented Sep 1, 2023

Uh oh!

orpiske commented Sep 1, 2023

Uh oh!

essobedo commented Sep 1, 2023

Uh oh!

orpiske commented Sep 1, 2023

Uh oh!

orpiske commented Sep 5, 2023 • 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!

3 participants

orpiske commented Sep 1, 2023 •

edited

Loading

orpiske commented Sep 5, 2023 •

edited

Loading