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

[CI] fix(*) ensure fake requests filter chains are freed #421

Closed
wants to merge 3 commits into from

Conversation

thibaultcha
Copy link
Member

A patch atop #420 for an underlying issue regarding fake requests "fake" filter chains.

hishamhm and others added 3 commits September 25, 2023 16:15
We were observing linear memory growth when running requests in low-isolation
modes.

The root cause comes from the fact that small allocations had instance-long
lifetimes (including, for example, the arrays for `args` and `rets` for each
Wasm call, causing the observable linear growth when running wrk).

Turns out that `ngx_pfree`, which was being used in the code to free `args` and
`rets`, does not free small allocations. It only frees large allocations; small
allocations are only freed when the pool itself is destroyed.

The solution is to use a request-lived pool for `pwctx`, so we don't hoard
small request-specific allocations over the lifetime of an instance.

We also change the setup of the `pwexec` pool so that it uses the `pwctx`
pool under all isolation modes, like it does for logs. Wasm isolation mode
should pertain only to the Wasm-accessible store, not to the temporary C-side
data structures, which we now free on each request.
Perform adjustment of arguments and return values in the stack
for the common case of functions with fewer than 10 arguments
and/or 10 returns.
Such as fake request passed to the FFI via the init_worker_by_lua fake
request.
@thibaultcha thibaultcha force-pushed the fix/memory-pool-fakereq branch from 0d63b5f to f7f9ca8 Compare September 27, 2023 01:20
@codecov
Copy link

codecov bot commented Sep 27, 2023

Codecov Report

Merging #421 (f7f9ca8) into main (f57914c) will decrease coverage by 0.11542%.
The diff coverage is 70.58824%.

Additional details and impacted files

Impacted file tree graph

@@                 Coverage Diff                 @@
##                main        #421         +/-   ##
===================================================
- Coverage   90.26802%   90.15260%   -0.11543%     
===================================================
  Files             46          46                 
  Lines           8395        8388          -7     
===================================================
- Hits            7578        7562         -16     
- Misses           817         826          +9     
Files Coverage Δ
src/common/lua/ngx_wasm_lua_ffi.c 90.54054% <100.00000%> (ø)
src/common/proxy_wasm/ngx_proxy_wasm.h 91.42857% <ø> (ø)
src/common/proxy_wasm/ngx_proxy_wasm.c 92.45283% <87.50000%> (-0.38425%) ⬇️
src/http/proxy_wasm/ngx_http_proxy_wasm.c 91.44737% <90.00000%> (-0.10193%) ⬇️
src/wasm/wrt/ngx_wrt_wasmtime.c 81.94444% <42.85714%> (-2.45272%) ⬇️

... and 1 file with indirect coverage changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants