fix memory leaks

[withinboredom]

There's a small ~800 byte memory leak on worker shutdown. This frees the request context once the thread is shut down.

This also addresses a memory leak by resources where we now delete any un-referenced resources after a request.

#1321

[AlliBalliBaba]

I also added a fix to content_type, that makes Fuzzing Tests currently fail on main.

While this variant makes tests green, there could be other types of streams that we would not want to destroy to avoid segfaults.

[withinboredom]

lol, my push got blocked and we did the same exact change... :D

[withinboredom]

I think this is good to go (we may have to just run it on a few projects to see if there are any new issues)?

Are there any objections to merging as-is? @dunglas / @AlliBalliBaba

[AlliBalliBaba]

This definitely also fixes the slowdown which probably comes from mapping over an increasingly big EG(regular_list).

This does not fix the reason why big MySQL queries are so much slower than big PostgreSQL queries though. After building and testing this branch, it's the same for worker and non-worker mode. It could be related to the ZTS TODO you mentioned. Might also be worth testing if the same thing happens when using the native MySQL driver.

[AlliBalliBaba]

This probably is semi-safe to merge. It would be nice to have some tests with resources that persist across requests.

[AlliBalliBaba]

I added a test, feel free to add more. Not sure what other types of streams exist.

[dunglas]

Impressive debugging session and collaboration!

Thank to both of you!!

[withinboredom]

This definitely also fixes the slowdown which probably comes from mapping over an increasingly big EG(regular_list).

That and GC pressure as well (more often GC means less time doing useful stuff).

[withinboredom]

This does not fix the reason why big MySQL queries are so much slower than big PostgreSQL queries though. After building and testing this branch, it's the same for worker and non-worker mode. It could be related to the ZTS TODO you mentioned. Might also be worth testing if the same thing happens when using the native MySQL driver.

Does disabling mysqlnd help any? mysqlnd is basically a connection pool for MySQL drivers and may be introducing contention. I'll try to stand up a test application as well. Also, you need to change some settings for PDO and MySQL:

• turn on buffered queries (to perform more than one query at a time)
• disable multiple queries per prepare (security; sql-injection)

There may be others, but I don't remember them off the top of my head and I can't remember if frameworks set this up for you.

[AlliBalliBaba]

Does disabling mysqlnd help any? mysqlnd is basically a connection pool for MySQL drivers and may be introducing contention. I'll try to stand up a test application as well. Also, you need to change some settings for PDO and MySQL:

Now that you mentioned ini settings, I tried mysqlnd.collect_statistics=Off and that actually allows doing 7x more big queries (1000 eloquent models). It's still not as fast as Postgres, but pretty close.

So I suspect this is due to a lock in mysqlnd statistics collection.

[AlliBalliBaba]

Haven't tested it, but my guess is it could be these lines that create a lock on every single database row read

https://github.com/php/php-src/blob/75d7684e9f6c7a883d93d02f67b6a8ed2ff6d091/ext/mysqlnd/mysqlnd_wireprotocol.c#L1544

https://github.com/php/php-src/blob/75d7684e9f6c7a883d93d02f67b6a8ed2ff6d091/ext/mysqlnd/mysqlnd_wireprotocol.c#L1630

So the slowdown gets noticeable as soon as many CPU cores are all busy only reading lines coming from MySQL.

[withinboredom]

Nice find!

[AlliBalliBaba]

I noticed that this branch can sometimes cause memory corruptions with lots of failed curl requests. Probably would be better to explicitly only clear the stream from the request body if possible.

[withinboredom]

This gets tricky and a route a spent quite a bit of time on yesterday but wasn't sure we needed to. We have no idea if a resource should be shared between requests or only in that request.

We need to hook into streams (set up a noop filter to configure a generational GC of sorts). If a resource hasn't been used for a few requests, or is closed, we remove it. If it is hot, we remove it from our checks. This isn't 100% perfect, but can be reasonably correct.

Alternatively, we hook into the AST (similar to opcache) and keep track of where the resource is created (global scope or not; and even this will be super complicated to track via realtime escape analysis). This is probably the "more correct" implementation but waaay more complicated -- and I'm not 100% sure it will work.

Another approach, the one I started implementing, is much simpler than either option: simply get the count of the resource list at the start of the request and only consider those that have a higher index at the end of the request. This is probably the least correct solution but is probably the simplest.

[AlliBalliBaba]

Hmm I'd usually expect it to be save to remove all unreferenced temporary streams. I'll also try if doing this at the start of the request instead of at the end will get rid of the error.