Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf: accelerate the creation of the consumer cache #11840

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

xuruidong
Copy link
Contributor

Description

In my case, there are approximately 17,000+ consumers. Any update from a consumer will trigger a long-tail request. I found the methods consumer.plugin and consumer.consumers_kv cost too much time.

Test steps

Update one consumer, send a request. Repeat 5 times.

Test result:

Before optimization:
unit: second

consumer.plugin consumer.consumers_kv
0.192824 3.209864
0.178377 2.837157
0.156748 3.573214
0.152296 2.720143
0.161006 2.530867

After optimization:
unit: second

consumer.plugin consumer.consumers_kv
0.050382 0.036338
0.021982 0.016851
0.023093 0.017966
0.022090 0.016934

Fixes # (issue)

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change
  • I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

Signed-off-by: xuruidong <xuruidong@gmail.com>
@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. performance generate flamegraph for the current PR labels Dec 18, 2024
@praswicaksono
Copy link

To purge local cache need to restart apisix?

@membphis
Copy link
Member

this is the core reason whey the new way is better performance.

use the cached data, reduce to recall secret.fetch_secrets

image

I think we can use lrucache, which is easier way, here is the code example:

local lrucache = core.lrucache.new({
    ttl = 300, count = 5000  -- we can change the `count` as we need, in your case, we may change it to more than 17,000+
})

local new_consumer, err = lrucache(consumer, nil,
                    secret.fetch_secrets, consumer.auth_conf, false)

I can not 100% confirm if it works, welcome to make a test in this way, many thx

@xuruidong
Copy link
Contributor Author

this is the core reason whey the new way is better performance.

use the cached data, reduce to recall secret.fetch_secrets

image I think we can use `lrucache`, which is easier way, here is the code example:
local lrucache = core.lrucache.new({
    ttl = 300, count = 5000  -- we can change the `count` as we need, in your case, we may change it to more than 17,000+
})

local new_consumer, err = lrucache(consumer, nil,
                    secret.fetch_secrets, consumer.auth_conf, false)

I can not 100% confirm if it works, welcome to make a test in this way, many thx

Let me make the ci happy first, and then I'll try your suggestion.

Signed-off-by: xuruidong <xuruidong@gmail.com>
@xuruidong
Copy link
Contributor Author

ping @membphis

conf_version = consumers.conf_version
}
end

local cached_consumer = consumers_id_cache[val.value.id]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not know why we have to add consumers_id_cache

it seems useless

new_consumer.auth_conf = secret.fetch_secrets(new_consumer.auth_conf, true,
new_consumer.auth_conf, "")
consumer_names[new_consumer.auth_conf[key_attr]] = new_consumer
local new_consumer = lru_cache(consumer.auth_conf[key_attr],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think: this should be enough, and it seems easier

Suggested change
local new_consumer = lru_cache(consumer.auth_conf[key_attr],
local new_consumer = lru_cache(consumer, nil, create_new_consumer, consumer)

do
local consumers_plugin_key_lrucache_tab = {}

local function create_new_consumer(consumer)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need a better name, it is not creating a new consumer

it should be transform or fill secret, what do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance generate flamegraph for the current PR size:M This PR changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants