Refactor newTopics to remove nested loops #4026
Conversation
921fb49 to
8b86d88
Compare
|
Thank you for contribution ! |
8b86d88 to
a12aceb
Compare
There was a problem hiding this comment.
Do you have proof how bad is a performance?
How many topics should we have to suffer from the logic we have so far?
I see that you have extracted a new local variable and you do have a new extra loop.
I'm not sure how that help with what we have so far.
I do see an improvement in the end of method though:
newTopicsMap.entrySet().removeIf(next -> !this.createOrModifyTopic.test(next.getValue()));
return new ArrayList<>(newTopicsMap.values());
And it feels like those duplicated TopicForRetryable instances also could be removed over here with that removeIf() callback.
Please, elaborate.
Replace O(n²) nested iteration with O(n) map-based lookups for filtering topics Use topicNameToMapKey to efficiently resolve duplicate topic names This improves application startup time and scalability when managing a large number of topics, while preserving the original filtering logic Resolves: spring-projects#4025 Signed-off-by: Choi Wang Gyu <dhkdrb897@gmail.com>
Replace manual Iterator loop with Collection.removeIf() to improve code readability and maintainability. The original implementation used a while loop with Iterator to remove entries that don't match the createOrModifyTopic predicate. While this worked, it required more boilerplate code and was less expressive. The new implementation uses the removeIf method introduced in Java 8, which directly expresses the filtering operation in a single line. This reduces method complexity and makes the code more idiomatic. Signed-off-by: Choi Wang Gyu <dhkdrb897@gmail.com>
a12aceb to
e0dadd5
Compare
I am attaching the results of the benchmark performance test. The above is before, and the below is after the code change |
| @@ -333,30 +332,22 @@ protected Collection<NewTopic> newTopics() { | |||
| Map<String, NewTopic> topicsForRetry = newTopicsMap.entrySet().stream() | |||
| .filter(entry -> entry.getValue() instanceof TopicForRetryable) | |||
| .collect(Collectors.toMap(Entry::getKey, Entry::getValue)); | |||
| Map<String, String> topicNameToMapKey = new HashMap<>(); | |||
There was a problem hiding this comment.
I still don't believe in this extra map.
More over its name does not reflect reality.
That this.applicationContext.getBeansOfType() returns for us a map of bean names to beans.
Doesn't look like we deal with bean names over here in this method at all.
So, we could simplify the logic just to deal with a List<NewTopic> in this method.
Then those topicsForRetry could be just topic names, since according to its logic we deliberately aware that those names are only for TopicForRetryable.
And so on from here, down to a single removeIf() in the end.
Refactored newTopics() to directly collect NewTopic and NewTopics beans into a single List<NewTopic>, avoiding unnecessary use of Map and bean names. TopicForRetryable instances are now removed if a regular NewTopic with the same name exists. This simplifies the logic and improves code readability. The final filtering is performed using removeIf for clarity and performance. Signed-off-by: Choi Wang Gyu <dhkdrb897@gmail.com>
| .forEach(wrapper -> newTopicsList.addAll(wrapper.getNewTopics())); | ||
|
|
||
| // Collect normal topic names to check against TopicForRetryable | ||
| Set<String> normalNames = newTopicsList.stream() |
There was a problem hiding this comment.
Just nit-pick: I don't think "normal" is a proper term to use.
And not only here.
It raises a question like what is the "abnormal"?
Is a TopicForRetryable abnormal?
I would suggest to go with a "regular" instead.
This way a TopicForRetryable is really "unregular" and that does not cause extra questions.
There was a problem hiding this comment.
Thank you very much!
I hadn't thought about it that way, but your point makes a lot of sense.
I'll update the code to reflect your feedback.
| newTopicsList.removeIf(nt -> nt instanceof TopicForRetryable && normalNames.contains(nt.name())); | ||
|
|
||
| // Apply predicate filter | ||
| newTopicsList.removeIf(nt -> !this.createOrModifyTopic.test(nt)); |
There was a problem hiding this comment.
Cannot we combine both conditions in the same removeIf()?
This way we would perform only one loop for removal logic.
There was a problem hiding this comment.
By the way, I'm curious—should these kinds of cases always be combined into one? I separated them intentionally for readability. In what situations is it better to merge them, and what are your thoughts on this?
There was a problem hiding this comment.
Right. I wouldn't merge then if you would not raise a performance concern 😄
Merge dual removeIf operations into a single filter condition. Rename normalNames to regularTopicNames for improved clarity. This change optimizes performance by reducing the number of collection iterations and improves code readability by using clearer naming. Signed-off-by: Choi Wang Gyu <dhkdrb897@gmail.com>
Replace O(n²) nested iteration with O(n) map-based lookups for filtering topics Use topicNameToMapKey to efficiently resolve duplicate topic names This improves application startup time and scalability when managing a large number of topics, while preserving the original filtering logic
Resolves: #4025