Added filter order optimizations #3389
Conversation
Signed-off-by: Calum Murray <cmurray@redhat.com>
knative-prow
bot
added
the
size/L
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Cali0707 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
knative-prow
bot
added
approved
|
/cc @pierDipi |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3389 +/- ##
============================================
- Coverage 61.48% 58.34% -3.14%
============================================
Files 181 91 -90
Lines 12356 9279 -3077
Branches 265 0 -265
============================================
- Hits 7597 5414 -2183
+ Misses 4159 3431 -728
+ Partials 600 434 -166
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
/hold |
knative-prow
bot
added
the
do-not-merge/hold
| import java.util.concurrent.locks.ReadWriteLock; | ||
| import org.slf4j.Logger; | ||
|
|
||
| public class FilterListOptimizer extends Thread { |
There was a problem hiding this comment.
A thread is way bigger than a go routing and during our scalability tests we reached the maximum thread count (which can't be easily increased in many platforms), so having more threads is a bit problematic until we have loom threads I don't really recommend increasing thread count (1 per consumer group is a lot)
There was a problem hiding this comment.
I believe what we might gain from runtime optimizations we will lose in higher memory usage and risk of blocking the Vertx event loop
| private final List<FilterCounter> filters; | ||
|
|
||
| private final ArrayBlockingQueue<Integer> indexSwapQueue; | ||
|
|
||
| private final FilterListOptimizer filterListOptimizer; | ||
|
|
||
| private final ReadWriteLock readWriteLock; |
There was a problem hiding this comment.
Generally, Vertx doesn't like locks and blocking operations, I'm pretty sure with this implementation we will block the event loop and that causes basically to block any event delivery for a particular trigger
Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
|
@pierDipi @matzew not sure if either of you have experience with Vert.x + JMH, but I am getting the following error while running benchmarks and am not sure how to fix it: |
Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
knative-prow-robot
added
the
needs-rebase
Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
knative-prow-robot
removed
the
needs-rebase
|
/unhold AnyFilterAllFilterI think these are reasonable tradeoffs, but if not let me know and I'm happy to close this PR :) Also, it is worth noting that the AllFilterBenchmarks don'tn test for a case when this re-ordering will lead to the largest speedup i.e. one non-matching filter at the end of the filters array, so we would see an even larger speedup there. |
knative-prow
bot
removed
the
do-not-merge/hold
|
/test |
|
@Cali0707: The
The following commands are available to trigger optional jobs:
Use
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/test reconciler-tests-keda |
|
@Cali0707: The following tests failed, say
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
Okay, with the benchmark logging issues fixed, I was able to benchmark this optimization and track the memory usage diff. The results were: So, on average this increased the memory usage of an any filter by approx. 400 bytes. For an all filter, this should be the same as the same code was used. IMO, this is a reasonable tradeoff for the large (100%+) throughput improvements |
|
This Pull Request is stale because it has been open for 90 days with |
github-actions
bot
added
lifecycle/stale
This PR brings the filter order optimizations from eventing core to the java filter implementation
Proposed Changes
FilterListOptimizerclass that handles the optimization loopFilterListOptimizerclass with theAnyFilterandAllFilterRelease Note
Docs