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

fix: reduce memory usage in client-h1 #3510

Merged
merged 1 commit into from
Aug 27, 2024

Conversation

Uzlopak
Copy link
Contributor

@Uzlopak Uzlopak commented Aug 26, 2024

Potentially fixes #2143 ?

tbh... I investigated the heap and saw alot of Timeouts (of our fast timers implementation) dangling(?) with onParserTimeout as callback. I guess when you run it for 6 minutes, like in the reproduction of the reported issue, some of those timeouts keep dangling and get not gc'ed?

Anyhow... this PR seems to reduce the memory usage anyway?!

So please review/take it with a grain of salt.

before:

aras@aras-Lenovo-Legion-5-17ARH05H:~/workspace/undici$ node --expose-gc test/fetch/fire-and-forget.js 
RSS 90 MB after 50 fetch() requests
RSS 94 MB after 100 fetch() requests
RSS 105 MB after 150 fetch() requests
RSS 119 MB after 200 fetch() requests
RSS 132 MB after 250 fetch() requests
RSS 143 MB after 300 fetch() requests
RSS 154 MB after 350 fetch() requests
RSS 157 MB after 400 fetch() requests
RSS 159 MB after 450 fetch() requests
RSS 159 MB after 500 fetch() requests
RSS 175 MB after 550 fetch() requests
RSS 179 MB after 600 fetch() requests
RSS 180 MB after 650 fetch() requests
RSS 181 MB after 700 fetch() requests
RSS 190 MB after 750 fetch() requests
RSS 200 MB after 800 fetch() requests
RSS 203 MB after 850 fetch() requests
RSS 204 MB after 900 fetch() requests
RSS 204 MB after 950 fetch() requests
RSS 204 MB after 1000 fetch() requests
RSS 205 MB after 1050 fetch() requests
RSS 204 MB after 1100 fetch() requests
RSS 206 MB after 1150 fetch() requests
RSS 206 MB after 1200 fetch() requests
RSS 208 MB after 1250 fetch() requests
RSS 218 MB after 1300 fetch() requests
RSS 220 MB after 1350 fetch() requests
RSS 223 MB after 1400 fetch() requests
RSS 223 MB after 1450 fetch() requests
RSS 223 MB after 1500 fetch() requests
RSS 224 MB after 1550 fetch() requests
RSS 223 MB after 1600 fetch() requests
RSS 224 MB after 1650 fetch() requests
RSS 224 MB after 1700 fetch() requests
RSS 225 MB after 1750 fetch() requests
RSS 228 MB after 1800 fetch() requests
RSS 229 MB after 1850 fetch() requests
RSS 231 MB after 1900 fetch() requests
RSS 231 MB after 1950 fetch() requests
RSS 231 MB after 2000 fetch() requests
RSS 231 MB after 2050 fetch() requests
RSS 231 MB after 2100 fetch() requests
RSS 231 MB after 2150 fetch() requests
RSS 231 MB after 2200 fetch() requests
RSS 232 MB after 2250 fetch() requests
RSS 231 MB after 2300 fetch() requests
RSS 232 MB after 2350 fetch() requests
RSS 232 MB after 2400 fetch() requests
RSS 232 MB after 2450 fetch() requests
RSS 232 MB after 2500 fetch() requests
RSS 232 MB after 2550 fetch() requests
RSS 232 MB after 2600 fetch() requests
RSS 233 MB after 2650 fetch() requests
RSS 233 MB after 2700 fetch() requests
RSS 233 MB after 2750 fetch() requests
RSS 232 MB after 2800 fetch() requests
RSS 233 MB after 2850 fetch() requests
RSS 233 MB after 2900 fetch() requests
RSS 233 MB after 2950 fetch() requests
RSS 233 MB after 3000 fetch() requests
RSS 233 MB after 3050 fetch() requests
RSS 234 MB after 3100 fetch() requests
RSS 235 MB after 3150 fetch() requests
RSS 235 MB after 3200 fetch() requests
RSS 235 MB after 3250 fetch() requests
RSS 234 MB after 3300 fetch() requests
RSS 236 MB after 3350 fetch() requests
RSS 236 MB after 3400 fetch() requests
RSS 237 MB after 3450 fetch() requests
RSS 237 MB after 3500 fetch() requests
RSS 236 MB after 3550 fetch() requests
RSS 238 MB after 3600 fetch() requests
RSS 238 MB after 3650 fetch() requests
RSS 239 MB after 3700 fetch() requests
RSS 239 MB after 3750 fetch() requests
RSS 238 MB after 3800 fetch() requests
RSS 240 MB after 3850 fetch() requests
RSS 240 MB after 3900 fetch() requests
RSS 240 MB after 3950 fetch() requests
RSS 240 MB after 4000 fetch() requests
RSS 240 MB after 4050 fetch() requests
RSS 241 MB after 4100 fetch() requests
RSS 242 MB after 4150 fetch() requests
RSS 242 MB after 4200 fetch() requests
RSS 242 MB after 4250 fetch() requests
RSS 237 MB after 4300 fetch() requests
RSS 237 MB after 4350 fetch() requests
RSS 237 MB after 4400 fetch() requests
RSS 237 MB after 4450 fetch() requests
RSS 241 MB after 4500 fetch() requests
RSS 241 MB after 4550 fetch() requests
RSS 241 MB after 4600 fetch() requests
RSS 241 MB after 4650 fetch() requests
RSS 242 MB after 4700 fetch() requests
RSS 242 MB after 4750 fetch() requests
RSS 242 MB after 4800 fetch() requests
RSS 242 MB after 4850 fetch() requests
RSS 242 MB after 4900 fetch() requests
RSS 242 MB after 4950 fetch() requests
RSS 242 MB after 5000 fetch() requests
✔ does not need the body to be consumed to continue (6085.97502ms)
ℹ tests 1
ℹ suites 0
ℹ pass 1
ℹ fail 0
ℹ cancelled 0
ℹ skipped 0
ℹ todo 0
ℹ duration_ms 6091.497357

after:

aras@aras-Lenovo-Legion-5-17ARH05H:~/workspace/undici$ node --expose-gc test/fetch/fire-and-forget.js 
RSS 90 MB after 50 fetch() requests
RSS 94 MB after 100 fetch() requests
RSS 105 MB after 150 fetch() requests
RSS 120 MB after 200 fetch() requests
RSS 133 MB after 250 fetch() requests
RSS 145 MB after 300 fetch() requests
RSS 156 MB after 350 fetch() requests
RSS 159 MB after 400 fetch() requests
RSS 160 MB after 450 fetch() requests
RSS 161 MB after 500 fetch() requests
RSS 176 MB after 550 fetch() requests
RSS 180 MB after 600 fetch() requests
RSS 180 MB after 650 fetch() requests
RSS 181 MB after 700 fetch() requests
RSS 181 MB after 750 fetch() requests
RSS 185 MB after 800 fetch() requests
RSS 186 MB after 850 fetch() requests
RSS 186 MB after 900 fetch() requests
RSS 187 MB after 950 fetch() requests
RSS 187 MB after 1000 fetch() requests
RSS 193 MB after 1050 fetch() requests
RSS 195 MB after 1100 fetch() requests
RSS 195 MB after 1150 fetch() requests
RSS 195 MB after 1200 fetch() requests
RSS 200 MB after 1250 fetch() requests
RSS 200 MB after 1300 fetch() requests
RSS 202 MB after 1350 fetch() requests
RSS 202 MB after 1400 fetch() requests
RSS 202 MB after 1450 fetch() requests
RSS 201 MB after 1500 fetch() requests
RSS 202 MB after 1550 fetch() requests
RSS 202 MB after 1600 fetch() requests
RSS 203 MB after 1650 fetch() requests
RSS 203 MB after 1700 fetch() requests
RSS 204 MB after 1750 fetch() requests
RSS 204 MB after 1800 fetch() requests
RSS 204 MB after 1850 fetch() requests
RSS 205 MB after 1900 fetch() requests
RSS 190 MB after 1950 fetch() requests
RSS 191 MB after 2000 fetch() requests
RSS 191 MB after 2050 fetch() requests
RSS 192 MB after 2100 fetch() requests
RSS 195 MB after 2150 fetch() requests
RSS 198 MB after 2200 fetch() requests
RSS 198 MB after 2250 fetch() requests
RSS 199 MB after 2300 fetch() requests
RSS 199 MB after 2350 fetch() requests
RSS 205 MB after 2400 fetch() requests
RSS 206 MB after 2450 fetch() requests
RSS 206 MB after 2500 fetch() requests
RSS 207 MB after 2550 fetch() requests
RSS 207 MB after 2600 fetch() requests
RSS 207 MB after 2650 fetch() requests
RSS 207 MB after 2700 fetch() requests
RSS 208 MB after 2750 fetch() requests
RSS 208 MB after 2800 fetch() requests
RSS 208 MB after 2850 fetch() requests
RSS 209 MB after 2900 fetch() requests
RSS 210 MB after 2950 fetch() requests
RSS 210 MB after 3000 fetch() requests
RSS 211 MB after 3050 fetch() requests
RSS 210 MB after 3100 fetch() requests
RSS 211 MB after 3150 fetch() requests
RSS 211 MB after 3200 fetch() requests
RSS 212 MB after 3250 fetch() requests
RSS 212 MB after 3300 fetch() requests
RSS 212 MB after 3350 fetch() requests
RSS 213 MB after 3400 fetch() requests
RSS 213 MB after 3450 fetch() requests
RSS 213 MB after 3500 fetch() requests
RSS 213 MB after 3550 fetch() requests
RSS 213 MB after 3600 fetch() requests
RSS 215 MB after 3650 fetch() requests
RSS 215 MB after 3700 fetch() requests
RSS 216 MB after 3750 fetch() requests
RSS 214 MB after 3800 fetch() requests
RSS 216 MB after 3850 fetch() requests
RSS 216 MB after 3900 fetch() requests
RSS 217 MB after 3950 fetch() requests
RSS 218 MB after 4000 fetch() requests
RSS 217 MB after 4050 fetch() requests
RSS 217 MB after 4100 fetch() requests
RSS 218 MB after 4150 fetch() requests
RSS 218 MB after 4200 fetch() requests
RSS 218 MB after 4250 fetch() requests
RSS 219 MB after 4300 fetch() requests
RSS 220 MB after 4350 fetch() requests
RSS 220 MB after 4400 fetch() requests
RSS 220 MB after 4450 fetch() requests
RSS 220 MB after 4500 fetch() requests
RSS 219 MB after 4550 fetch() requests
RSS 221 MB after 4600 fetch() requests
RSS 221 MB after 4650 fetch() requests
RSS 221 MB after 4700 fetch() requests
RSS 221 MB after 4750 fetch() requests
RSS 221 MB after 4800 fetch() requests
RSS 222 MB after 4850 fetch() requests
RSS 222 MB after 4900 fetch() requests
RSS 222 MB after 4950 fetch() requests
RSS 223 MB after 5000 fetch() requests
✔ does not need the body to be consumed to continue (6300.876627ms)
ℹ tests 1
ℹ suites 0
ℹ pass 1
ℹ fail 0
ℹ cancelled 0
ℹ skipped 0
ℹ todo 0
ℹ duration_ms 6306.996567

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Aug 26, 2024

If you modify the fire-and-forget test to do more fetches, then the result is more clear?!

before:
RSS 262 MB after 10000 fetch() requests

after:
RSS 226 MB after 10000 fetch() requests

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@Uzlopak Uzlopak merged commit b725457 into nodejs:main Aug 27, 2024
32 checks passed
@Uzlopak Uzlopak deleted the fix-memleak-on-parser-timeout branch August 27, 2024 06:26
@mcollina mcollina changed the title fix: avoid memoryleak in client-h1 fix: reduce memory usage in client-h1 Aug 27, 2024
@mcollina
Copy link
Member

I've amended the title, as there is no actual leak here.

@trivikr

This comment was marked as outdated.

github-actions bot pushed a commit that referenced this pull request Oct 3, 2024
Uzlopak added a commit that referenced this pull request Oct 3, 2024
(cherry picked from commit b725457)

Co-authored-by: Aras Abbasi <[email protected]>
@github-actions github-actions bot mentioned this pull request Dec 3, 2024
This was referenced Dec 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Memory leak with native fetch on Node v19+
5 participants