filters/auth: use sync.Map for tokeninfo cache #3267

AlexanderYastrebov · 2024-10-09T10:44:31Z

use sync.Map for tokeninfo cache to avoid synchronizing all callers on a single mutex
evict stale entries periodically instead of least recently used
store token expiration time instead of creation time

                                                  │      master      │                HEAD                 │
                                                  │      sec/op      │   sec/op     vs base                │
TokeninfoCache/tokens=1,cacheSize=1,p=0-8               275.5n ±  6%   170.1n ± 4%  -38.26% (p=0.000 n=10)
TokeninfoCache/tokens=2,cacheSize=2,p=0-8               492.9n ± 21%   176.8n ± 2%  -64.12% (p=0.000 n=10)
TokeninfoCache/tokens=100,cacheSize=100,p=0-8           455.9n ±  7%   165.5n ± 1%  -63.70% (p=0.000 n=10)
TokeninfoCache/tokens=100,cacheSize=100,p=10000-8       593.4n ±  4%   179.8n ± 4%  -69.71% (p=0.000 n=10)
TokeninfoCache/tokens=4,cacheSize=2,p=0-8           2571424.0n ±  0%   149.7n ± 3%  -99.99% (p=0.000 n=10)
TokeninfoCache/tokens=100,cacheSize=10,p=0-8        2579227.5n ±  0%   139.3n ± 1%  -99.99% (p=0.000 n=10)
geomean                                                 7.903µ         162.9n       -97.94%

                                                  │   master   │                  HEAD                   │
                                                  │    B/op    │    B/op      vs base                    │
TokeninfoCache/tokens=1,cacheSize=1,p=0-8           344.0 ± 0%    344.0 ± 0%          ~ (p=1.000 n=10) ¹
TokeninfoCache/tokens=2,cacheSize=2,p=0-8           344.0 ± 0%    344.0 ± 0%          ~ (p=1.000 n=10) ¹
TokeninfoCache/tokens=100,cacheSize=100,p=0-8       344.0 ± 0%    344.0 ± 0%          ~ (p=1.000 n=10) ¹
TokeninfoCache/tokens=100,cacheSize=100,p=10000-8   368.0 ± 1%    350.0 ± 0%     -4.89% (p=0.000 n=10)
TokeninfoCache/tokens=4,cacheSize=2,p=0-8           27.00 ± 0%   344.00 ± 0%  +1174.07% (p=0.000 n=10)
TokeninfoCache/tokens=100,cacheSize=10,p=0-8        27.00 ± 7%   344.00 ± 0%  +1174.07% (p=0.000 n=10)
geomean                                             149.0         345.0        +131.62%
¹ all samples are equal

                                                  │    master    │              HEAD              │
                                                  │  allocs/op   │ allocs/op   vs base            │
TokeninfoCache/tokens=1,cacheSize=1,p=0-8           3.000 ± 0%     3.000 ± 0%  ~ (p=1.000 n=10) ¹
TokeninfoCache/tokens=2,cacheSize=2,p=0-8           3.000 ± 0%     3.000 ± 0%  ~ (p=1.000 n=10) ¹
TokeninfoCache/tokens=100,cacheSize=100,p=0-8       3.000 ± 0%     3.000 ± 0%  ~ (p=1.000 n=10) ¹
TokeninfoCache/tokens=100,cacheSize=100,p=10000-8   3.000 ± 0%     3.000 ± 0%  ~ (p=1.000 n=10) ¹
TokeninfoCache/tokens=4,cacheSize=2,p=0-8           0.000 ± 0%     3.000 ± 0%  ? (p=0.000 n=10)
TokeninfoCache/tokens=100,cacheSize=10,p=0-8        0.000 ± 0%     3.000 ± 0%  ? (p=0.000 n=10)
geomean                                                        ²   3.000       ?
¹ all samples are equal
² summaries must be >0 to compute geomean

RomanZavodskikh · 2024-10-09T11:10:42Z

filters/auth/tokeninfocache.go

-		info:      info,
-		href:      c.history.PushFront(token),
+func (c *tokeninfoCache) evictLoop() {
+	ticker := time.NewTicker(time.Minute)


Does it make sense this time.Minute configurable at the next PR?

I am not sure which other value would be good.

RomanZavodskikh · 2024-10-09T11:20:51Z

filters/auth/tokeninfocache.go

-		mu    sync.Mutex
-		cache map[string]*entry
-		// least recently used token at the end
-		history *list.List


I see you would like to keep tokeninfo filter lock-free, that is why you are thinking about not keeping history any more.

However, there is a data structure which acts like a list and has lock-free implementations (at least in theory). https://en.wikipedia.org/wiki/Skip_list

Do you think it makes sense to try to obtain this data structure to have lock-free history access and not evict random items?

It was used to evict oldest entries when cache grows over the limit.
This change simplifies this by simply removing random items if number of cached items is over the size limit.
It also adds a metric for monitoring.

In production setup one should have cache size set large enough such that entries are never evicted due to overflow. This way there is no need to complicate eviction algorithm, keep access history or use clever datastructures.

filters/auth/tokeninfocache.go

filters/auth/authclient.go

filters/auth/tokeninfocache_test.go

szuecs

I hope my review makes sense.
Happy to see it happen!

filters/auth/tokeninfocache.go

* use sync.Map for tokeninfo cache to avoid synchronizing all callers on a single mutex * evict stale entries periodically instead of least recently used * store token expiration time instead of creation time ``` │ master │ HEAD │ │ sec/op │ sec/op vs base │ TokeninfoCache/tokens=1,cacheSize=1,p=0-8 275.5n ± 6% 170.1n ± 4% -38.26% (p=0.000 n=10) TokeninfoCache/tokens=2,cacheSize=2,p=0-8 492.9n ± 21% 176.8n ± 2% -64.12% (p=0.000 n=10) TokeninfoCache/tokens=100,cacheSize=100,p=0-8 455.9n ± 7% 165.5n ± 1% -63.70% (p=0.000 n=10) TokeninfoCache/tokens=100,cacheSize=100,p=10000-8 593.4n ± 4% 179.8n ± 4% -69.71% (p=0.000 n=10) TokeninfoCache/tokens=4,cacheSize=2,p=0-8 2571424.0n ± 0% 149.7n ± 3% -99.99% (p=0.000 n=10) TokeninfoCache/tokens=100,cacheSize=10,p=0-8 2579227.5n ± 0% 139.3n ± 1% -99.99% (p=0.000 n=10) geomean 7.903µ 162.9n -97.94% │ master │ HEAD │ │ B/op │ B/op vs base │ TokeninfoCache/tokens=1,cacheSize=1,p=0-8 344.0 ± 0% 344.0 ± 0% ~ (p=1.000 n=10) ¹ TokeninfoCache/tokens=2,cacheSize=2,p=0-8 344.0 ± 0% 344.0 ± 0% ~ (p=1.000 n=10) ¹ TokeninfoCache/tokens=100,cacheSize=100,p=0-8 344.0 ± 0% 344.0 ± 0% ~ (p=1.000 n=10) ¹ TokeninfoCache/tokens=100,cacheSize=100,p=10000-8 368.0 ± 1% 350.0 ± 0% -4.89% (p=0.000 n=10) TokeninfoCache/tokens=4,cacheSize=2,p=0-8 27.00 ± 0% 344.00 ± 0% +1174.07% (p=0.000 n=10) TokeninfoCache/tokens=100,cacheSize=10,p=0-8 27.00 ± 7% 344.00 ± 0% +1174.07% (p=0.000 n=10) geomean 149.0 345.0 +131.62% ¹ all samples are equal │ master │ HEAD │ │ allocs/op │ allocs/op vs base │ TokeninfoCache/tokens=1,cacheSize=1,p=0-8 3.000 ± 0% 3.000 ± 0% ~ (p=1.000 n=10) ¹ TokeninfoCache/tokens=2,cacheSize=2,p=0-8 3.000 ± 0% 3.000 ± 0% ~ (p=1.000 n=10) ¹ TokeninfoCache/tokens=100,cacheSize=100,p=0-8 3.000 ± 0% 3.000 ± 0% ~ (p=1.000 n=10) ¹ TokeninfoCache/tokens=100,cacheSize=100,p=10000-8 3.000 ± 0% 3.000 ± 0% ~ (p=1.000 n=10) ¹ TokeninfoCache/tokens=4,cacheSize=2,p=0-8 0.000 ± 0% 3.000 ± 0% ? (p=0.000 n=10) TokeninfoCache/tokens=100,cacheSize=10,p=0-8 0.000 ± 0% 3.000 ± 0% ? (p=0.000 n=10) geomean ² 3.000 ? ¹ all samples are equal ² summaries must be >0 to compute geomean ``` Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>

MustafaSaber · 2025-01-07T09:06:33Z

👍

AlexanderYastrebov · 2025-01-07T09:45:44Z

👍

AlexanderYastrebov added the major moderate risk, for example new API, small filter changes that have no risk like refactoring or logs label Oct 9, 2024

AlexanderYastrebov force-pushed the filters/auth/tokeninfocache-syncmap branch from 67cd561 to 813bf2a Compare October 9, 2024 11:01

RomanZavodskikh reviewed Oct 9, 2024

View reviewed changes

MustafaSaber reviewed Oct 9, 2024

View reviewed changes

filters/auth/tokeninfocache.go Show resolved Hide resolved

szuecs reviewed Oct 9, 2024

View reviewed changes

filters/auth/tokeninfocache.go Outdated Show resolved Hide resolved

szuecs reviewed Oct 9, 2024

View reviewed changes

filters/auth/authclient.go Show resolved Hide resolved

RomanZavodskikh reviewed Oct 9, 2024

View reviewed changes

filters/auth/tokeninfocache_test.go Show resolved Hide resolved

AlexanderYastrebov force-pushed the filters/auth/tokeninfocache-syncmap branch from 813bf2a to 818dd4d Compare October 10, 2024 11:22

AlexanderYastrebov force-pushed the filters/auth/tokeninfocache-syncmap branch from 818dd4d to c429f5e Compare January 3, 2025 16:17

szuecs reviewed Jan 5, 2025

View reviewed changes

filters/auth/tokeninfocache.go Show resolved Hide resolved

filters/auth/tokeninfocache.go Outdated Show resolved Hide resolved

AlexanderYastrebov force-pushed the filters/auth/tokeninfocache-syncmap branch from c429f5e to 294eaa7 Compare January 7, 2025 08:52

AlexanderYastrebov merged commit 94719ea into master Jan 7, 2025
14 checks passed

AlexanderYastrebov deleted the filters/auth/tokeninfocache-syncmap branch January 7, 2025 09:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

filters/auth: use sync.Map for tokeninfo cache #3267

filters/auth: use sync.Map for tokeninfo cache #3267

AlexanderYastrebov commented Oct 9, 2024

RomanZavodskikh Oct 9, 2024

AlexanderYastrebov Oct 9, 2024

RomanZavodskikh Oct 9, 2024

AlexanderYastrebov Oct 9, 2024 •

edited

Loading

szuecs left a comment

MustafaSaber commented Jan 7, 2025

AlexanderYastrebov commented Jan 7, 2025

filters/auth: use sync.Map for tokeninfo cache #3267

filters/auth: use sync.Map for tokeninfo cache #3267

Conversation

AlexanderYastrebov commented Oct 9, 2024

RomanZavodskikh Oct 9, 2024

Choose a reason for hiding this comment

AlexanderYastrebov Oct 9, 2024

Choose a reason for hiding this comment

RomanZavodskikh Oct 9, 2024

Choose a reason for hiding this comment

AlexanderYastrebov Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

szuecs left a comment

Choose a reason for hiding this comment

MustafaSaber commented Jan 7, 2025

AlexanderYastrebov commented Jan 7, 2025

AlexanderYastrebov Oct 9, 2024 •

edited

Loading