Aias00 commented on code in PR #6336:
URL: https://github.com/apache/shenyu/pull/6336#discussion_r4071035083
##########
shenyu-plugin/shenyu-plugin-base/src/main/java/org/apache/shenyu/plugin/base/AbstractShenyuPlugin.java:
##########
@@ -120,22 +115,37 @@ public Mono<Void> execute(final ServerWebExchange
exchange, final ShenyuPluginCh
printLog(rule, pluginName);
return doExecute(exchange, chain, selectorData, rule);
}
- // lru map as L1 cache,the cache is enabled by default.
- // if the L1 cache fails to hit, using L2 cache based on trie cache.
- // if the L2 cache fails to hit, execute default strategy.
- RuleData ruleData = obtainRuleDataCacheIfEnabled(path);
- if (Objects.nonNull(ruleData) && Objects.isNull(ruleData.getId())) {
+ RuleData ruleData = twoLevelCacheLookupRule(exchange, rules, path);
+ if (Objects.isNull(ruleData) || StringUtils.isBlank(ruleData.getId()))
{
return handleRuleIfNull(pluginName, exchange, chain);
}
- if (Objects.isNull(ruleData)) {
- ruleData = defaultMatchRule(exchange, rules, path);
- if (Objects.isNull(ruleData)) {
- return handleRuleIfNull(pluginName, exchange, chain);
- }
- }
printLog(ruleData, pluginName);
return doExecute(exchange, chain, selectorData, ruleData);
}
+
+ private SelectorData twoLevelCacheLookupSelector(final ServerWebExchange
exchange,
Review Comment:
Heads up: this helper is now redundant with what `master` already does
inline.
When this PR was opened (2026-04-30), `execute()` did:
```java
selectorData = defaultMatchSelector(exchange, selectors, path); //
unconditional
if (Objects.isNull(selectorData)) { return handleSelectorIfNull(...); }
```
so a positive L1 hit was thrown away and re-matched every request — that was
the real bug you were fixing here.
But #7055 ("Fixed selector and rule match bug", merged 2026-09-11) already
changed it to:
```java
if (Objects.isNull(selectorData)) {
selectorData = defaultMatchSelector(exchange, selectors, path);
...
}
```
This branch merged `master` on 2026-09-22, so it now carries that fix, and
`twoLevelCacheLookupSelector` re-implements exactly the semantics `execute()`
already has. Same for `twoLevelCacheLookupRule` — the only surviving delta is
`Objects.isNull(id)` -> `StringUtils.isBlank(id)`.
Not a defect, but please decide explicitly: either drop these two wrappers
and keep only the `isBlank` tightening (smallest possible diff), or keep them
purely as named helpers and say so in the PR description. Right now the commit
message ("Make selector/rule L1 match cache effective") describes a bug that is
already fixed on `master`, which will confuse whoever reads the git history
later.
##########
shenyu-plugin/shenyu-plugin-base/src/main/java/org/apache/shenyu/plugin/base/AbstractShenyuPlugin.java:
##########
@@ -158,43 +168,52 @@ private RuleData obtainRuleDataCacheIfEnabled(final
String path) {
}
private void cacheSelectorData(final String path, final SelectorData
selectorData) {
- if (Boolean.FALSE.equals(selectorMatchConfig.getCache().getEnabled())
|| Objects.isNull(selectorData)
- || Boolean.TRUE.equals(selectorData.getMatchRestful())) {
- return;
- }
- int initialCapacity =
selectorMatchConfig.getCache().getInitialCapacity();
- long maximumSize = selectorMatchConfig.getCache().getMaximumSize();
- if (StringUtils.isBlank(selectorData.getId())) {
- MatchDataCache.getInstance().cacheSelectorData(path, selectorData,
initialCapacity, maximumSize);
- return;
- }
- List<ConditionData> conditionList = selectorData.getConditionList();
- if (CollectionUtils.isNotEmpty(conditionList)) {
- boolean isUriCondition = conditionList.stream().allMatch(v ->
URI_CONDITION_TYPE.equals(v.getParamType()));
- if (isUriCondition) {
- MatchDataCache.getInstance().cacheSelectorData(path,
selectorData, initialCapacity, maximumSize);
- }
- }
+ cacheMatchData(
+ path,
+ selectorData,
+ selectorMatchConfig.getCache(),
+ SelectorData::getId,
+ SelectorData::getMatchRestful,
+ SelectorData::getConditionList,
+ MatchDataCache.getInstance()::cacheSelectorData);
}
-
+
private void cacheRuleData(final String path, final RuleData ruleData) {
- // if the ruleCache is disabled or rule data is null, not cache rule
data.
- if (Boolean.FALSE.equals(ruleMatchConfig.getCache().getEnabled()) ||
Objects.isNull(ruleData)
- || Boolean.TRUE.equals(ruleData.getMatchRestful())) {
+ cacheMatchData(
+ path,
+ ruleData,
+ ruleMatchConfig.getCache(),
+ RuleData::getId,
+ RuleData::getMatchRestful,
+ RuleData::getConditionDataList,
+ MatchDataCache.getInstance()::cacheRuleData);
+ }
+
+ private <T> void cacheMatchData(final String path,
+ final T data,
+ final ShenyuConfig.MatchCacheConfig
cacheConfig,
+ final Function<T, String> idGetter,
+ final Function<T, Boolean> restfulGetter,
+ final Function<T, List<ConditionData>>
conditionGetter,
+ final CacheWriter<T> cacheWriter) {
+ // if the cache is disabled or data is null or matchRestful, do not
cache.
+ if (Boolean.FALSE.equals(cacheConfig.getEnabled())
+ || Objects.isNull(data)
+ || Boolean.TRUE.equals(restfulGetter.apply(data))) {
return;
}
- int initialCapacity = ruleMatchConfig.getCache().getInitialCapacity();
- long maximumSize = ruleMatchConfig.getCache().getMaximumSize();
- if (StringUtils.isBlank(ruleData.getId())) {
- MatchDataCache.getInstance().cacheRuleData(path, ruleData,
initialCapacity, maximumSize);
+ final int initialCapacity = cacheConfig.getInitialCapacity();
+ final long maximumSize = cacheConfig.getMaximumSize();
+ // null-id sentinel: always cached to short-circuit the next miss.
+ if (StringUtils.isBlank(idGetter.apply(data))) {
Review Comment:
The negative-sentinel predicate is not the same on the write/read path and
the eviction path.
- Here (and in `execute()`, after this PR): `StringUtils.isBlank(id)` ->
treat as "cached miss", short-circuit to `handleSelectorIfNull` /
`handleRuleIfNull`.
- `MatchDataCache#removeEmptySelectorData` (line 95) and
`#removeEmptyRuleData` (line 190): `Objects.isNull(entry.getValue().getId())`
-> evict.
So an entry whose id is `""` (or whitespace) is written as a negative
sentinel, is served as a negative sentinel, and is **never** removed when the
plugin/selector data changes — it can only disappear via LRU eviction.
`execute()` is what turns `isBlank` into a *read* predicate for the rule path
too, so this PR widens an existing gap rather than closing it.
Practically the id comes from admin and is never blank, so the blast radius
is near zero. Still, the sentinel should be defined once. Either:
```java
// MatchDataCache
pathSelectorCache.entrySet().removeIf(entry ->
StringUtils.isBlank(entry.getValue().getId()));
```
or narrow both sides back to `Objects.isNull(...)`. Please pick one and keep
`cacheMatchData`, `execute()` and `removeEmpty*Data` consistent.
##########
shenyu-plugin/shenyu-plugin-base/src/main/java/org/apache/shenyu/plugin/base/AbstractShenyuPlugin.java:
##########
@@ -231,69 +250,83 @@ protected Mono<Void> handleRuleIfNull(final String
pluginName, final ServerWebEx
}
private Pair<Boolean, SelectorData> matchSelector(final ServerWebExchange
exchange, final Collection<SelectorData> selectors) {
- List<SelectorData> filterCollectors = selectors.stream()
- .filter(selector -> selector.getEnabled() &&
filterSelector(selector, exchange))
- .distinct()
- .collect(Collectors.toList());
- if (filterCollectors.size() > 1) {
- return Pair.of(Boolean.FALSE, manyMatchSelector(filterCollectors));
- } else {
- return Pair.of(Boolean.TRUE,
filterCollectors.stream().findFirst().orElse(null));
+ // hot path: prefer plain loop over stream to reduce allocations.
+ // de-duplicate via LinkedHashSet to preserve original semantics
(equals/hashCode based).
+ Set<SelectorData> matched = null;
Review Comment:
Two things here:
1. The comment says "prefer plain loop over stream to reduce allocations",
but on the actual hot path (exactly one match) this allocates *more*, not less:
`new LinkedHashSet<>(4)` builds a `HashMap` with an 8-slot table plus an
`Entry`, and then `matched.iterator()` allocates an iterator. The old
`distinct()` already did hash-based dedup internally, so dedup cost is
unchanged — the only thing removed is the stream pipeline. And for the
multi-match case `new ArrayList<>(matched)` is a second allocation that did not
exist before. Either drop the perf claim from the comment, or back it with a
JMH number.
2. `matched.isEmpty()` is unreachable — `matched` is only assigned inside
the `if (...)` right before an `add`, so it can never be empty when non-null.
`Objects.isNull(matched)` alone is enough.
Same two points apply to `matchRule` below.
##########
shenyu-plugin/shenyu-plugin-base/src/test/java/org/apache/shenyu/plugin/base/AbstractShenyuPluginTest.java:
##########
@@ -254,6 +254,84 @@ public void clear() {
MatchDataCache.getInstance().cleanRuleDataData();
}
+ /**
+ * Test L1 cache hit for selector: return the cached selector directly
without L2 matching.
+ */
+ @Test
+ public void executeSelectorL1CacheHitTest() {
Review Comment:
This test does not actually pin the L1 short-circuit.
You warm L1 with the *same* selector that L2 matching would return, so if
the L1 lookup were removed entirely the test would still pass — L2 would
produce an identical `SelectorData` and `doExecute` would be called with the
same arguments.
I verified this: I swapped in `master`'s `AbstractShenyuPlugin.java` (61
sources recompiled), kept your new tests, and ran `AbstractShenyuPluginTest` —
`Tests run: 12, Failures: 0, Errors: 0`. All four new tests pass unchanged
against the pre-PR implementation.
To actually assert "L1 hit is returned without going to L2", make L1 and L2
disagree, e.g.:
```java
// warm L1 with a *different* selector than the one L2 would match
SelectorData cached =
SelectorData.builder().id("cached-id").pluginName("SHENYU")
.enabled(true).matchRestful(false).build();
MatchDataCache.getInstance().cacheSelectorData("/http/SHENYU/SHENYU",
cached, 100, 100);
...
verify(testShenyuPlugin).doExecute(exchange, shenyuPluginChain, cached,
ruleData);
```
or assert the negative direction with `verify(testShenyuPlugin, never())` on
the matching path. Same applies to `executeRuleL1CacheHitTest`. The two
`*EmptySentinelTest` cases do have real value (they cover the null-id
short-circuit) — it is the two positive-hit cases that are no-ops.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
