You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by "AlinsRan (via GitHub)" <gi...@apache.org> on 2026/07/29 13:26:49 UTC
[PR] perf(plugin): avoid deep-copying the plugins subtree in merge_consumer_route [apisix]
AlinsRan opened a new pull request, #13757:
URL: https://github.com/apache/apisix/pull/13757
### Description
`merge_consumer_route` runs on every request that hits a consumer, and it deep-copies the whole `route_conf`:
```lua
local new_route_conf = core.table.deepcopy(route_conf)
```
then overwrites `new_route_conf.value.plugins[name] = conf` entry by entry. Deep-copying the `value.plugins` subtree — usually the largest part of a route — is wasted work, since every entry that matters is replaced right after.
This adds an `opts.shallow_prefix` option to `core.table.deepcopy` that shallow-copies members whose parent path starts with the given prefix, and uses `{ shallow_prefix = "self.value.plugins" }` in `merge_consumer_route`.
It is semantically equivalent: the `plugins` container is still a fresh table (so overwriting keys does not touch the original), and only the plugin confs that are *not* overwritten stay as shared references — and those are read-only here (`merge_consumer_route` always replaces a key, never mutates a shared conf in place).
### Which issue(s) this PR fixes
Removes one deep copy of the largest part of a route on the consumer-auth hot path.
### Checklist
- [x] I have explained the need for this PR and the problem it solves
- [x] I have explained the changes or the new features added to this PR
- [x] I have added tests corresponding to this change (`t/core/table.t` TEST 13)
- [x] I have updated the documentation accordingly
- [ ] I have verified locally (full CI covers the rest)
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] perf(plugin): avoid deep-copying the plugins subtree in merge_consumer_route [apisix]
Posted by "AlinsRan (via GitHub)" <gi...@apache.org>.
AlinsRan closed pull request #13757: perf(plugin): avoid deep-copying the plugins subtree in merge_consumer_route
URL: https://github.com/apache/apisix/pull/13757
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(plugin): preserve plugin-conf state across consumer route merge [apisix]
Posted by "AlinsRan (via GitHub)" <gi...@apache.org>.
AlinsRan merged PR #13757:
URL: https://github.com/apache/apisix/pull/13757
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(plugin): preserve plugin-conf state across consumer route merge [apisix]
Posted by "AlinsRan (via GitHub)" <gi...@apache.org>.
AlinsRan commented on PR #13757:
URL: https://github.com/apache/apisix/pull/13757#issuecomment-5118823898
Correcting my earlier close comment — it was wrong. I closed this thinking the only motivation (a healthcheck rebuild) had been refactored away. But the healthcheck path was just one instance of a broader issue: plugins cache request-time state on their conf (DNS resolution in `ai-proxy-multi`, `_nodes_ver` in `upstream.lua`, probed `_version` in `elasticsearch-logger`), and the `value.plugins` deep-copy in `merge_consumer_route` discards that state on the consumer path. Reframing as the correctness fix it actually is (title changed from perf → fix) and reopening.
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(plugin): preserve plugin-conf state across consumer route merge [apisix]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13757:
URL: https://github.com/apache/apisix/pull/13757#discussion_r3679932509
##########
apisix/plugin.lua:
##########
@@ -818,7 +818,10 @@ local function merge_consumer_route(route_conf, consumer_conf, consumer_group_co
return route_conf
end
- local new_route_conf = core.table.deepcopy(route_conf)
+ -- the plugins subtree is fully overwritten below, so there is no need to
+ -- deep-copy it; shallow-copy that subtree to avoid the wasted work
+ local new_route_conf = core.table.deepcopy(route_conf,
+ { shallow_prefix = "self.value.plugins" })
Review Comment:
The new deepcopy option here is not just an optimization: the comment says the plugins subtree is "fully overwritten" and implies deep-copy is unnecessary, but the key reason is preserving plugin-conf identity (cached state) while still copying the `plugins` container table so overwrites don’t mutate the original route config.
##########
apisix/core/table.lua:
##########
@@ -130,7 +131,8 @@ do
copied[orig] = copy
for orig_key, orig_value in pairs(orig) do
local path = parent .. "." .. tostring(orig_key)
- if opts and array_find(opts.shallows, path) then
+ if opts and (array_find(opts.shallows, path) or
+ (opts.shallow_prefix and str_has_prefix(parent, opts.shallow_prefix))) then
copy[orig_key] = orig_value
Review Comment:
`shallow_prefix` matching uses a raw string prefix (`has_prefix(parent, shallow_prefix)`), which can unintentionally match sibling paths like `self.value.plugins2` when `shallow_prefix` is `self.value.plugins`. Also, `core.string.has_prefix` throws if `shallow_prefix` is not a string, while `opts.shallows` is currently tolerant of wrong types; the new option should be similarly defensive.
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] perf(plugin): avoid deep-copying the plugins subtree in merge_consumer_route [apisix]
Posted by "AlinsRan (via GitHub)" <gi...@apache.org>.
AlinsRan commented on PR #13757:
URL: https://github.com/apache/apisix/pull/13757#issuecomment-5118692685
Closing this — after review it does not hold up, and I'd rather withdraw it than waste maintainer time.
I originally framed it as a hot-path perf win, but `merge_consumer_route` is wrapped in an lrucache (`merged_route`, keyed by route+consumer+conf_version), so the deep copy only runs on cache miss, not per request — the perf benefit is negligible.
Its real origin was a bugfix: a downstream healthcheck implementation keyed a checker on the plugin-conf object identity, so deep-copying `value.plugins` in `merge_consumer_route` produced a fresh object every time and made the checker rebuild. But that healthcheck path has since been refactored to key on stable identifiers (resource key / version / target ip:port:hostname) instead of conf identity, so the deep copy no longer triggers any rebuild.
With neither the perf nor the correctness rationale standing, `opts.shallow_prefix` is just added API surface with a shared-reference footgun and no real benefit. Sorry for the noise.
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(plugin): preserve plugin-conf state across consumer route merge [apisix]
Posted by "AlinsRan (via GitHub)" <gi...@apache.org>.
AlinsRan commented on code in PR #13757:
URL: https://github.com/apache/apisix/pull/13757#discussion_r3687005926
##########
apisix/core/table.lua:
##########
@@ -130,7 +131,8 @@ do
copied[orig] = copy
for orig_key, orig_value in pairs(orig) do
local path = parent .. "." .. tostring(orig_key)
- if opts and array_find(opts.shallows, path) then
+ if opts and (array_find(opts.shallows, path) or
+ (opts.shallow_prefix and str_has_prefix(parent, opts.shallow_prefix))) then
copy[orig_key] = orig_value
Review Comment:
Fixed the boundary in 6eee234: the match now has to stop at a path separator, so `self.value.plugins` no longer claims a sibling like `self.value.plugins2`. `t/core/table.t` TEST 14 covers it (it returns `true` for the sibling before the fix, `false` after). No live caller was affected — the only `plugins`-ish sibling under `self.value` is `plugin_config_id`, which diverges before the prefix ends.
On the type tolerance: I would rather keep `has_prefix` raising here. `opts.shallows` is only tolerant because `array_find` returns `nil` for a non-table, i.e. a wrong type is silently treated as "not configured" — for `shallow_prefix` that would silently disable the sharing and bring back the state loss this PR fixes, which is much harder to notice than an error on the first request.
##########
apisix/plugin.lua:
##########
@@ -818,7 +818,10 @@ local function merge_consumer_route(route_conf, consumer_conf, consumer_group_co
return route_conf
end
- local new_route_conf = core.table.deepcopy(route_conf)
+ -- the plugins subtree is fully overwritten below, so there is no need to
+ -- deep-copy it; shallow-copy that subtree to avoid the wasted work
+ local new_route_conf = core.table.deepcopy(route_conf,
+ { shallow_prefix = "self.value.plugins" })
Review Comment:
Right, and the comment was wrong on the facts too — the merge only overwrites the plugin names carried by the consumer/consumer group, so a route-only plugin is never overwritten, and those are exactly the confs that need to keep their identity. Rewritten in 6eee234 to state the real reason (preserve cached plugin state; the `plugins` container is still a fresh table so the merge does not mutate the original route conf).
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org