RejectedSoftware Forums

On Thu, 12 Nov 2015 08:32:29 +0100, Sönke Ludwig wrote:

Am 21.10.2015 um 18:33 schrieb Márcio Martins:

Hi!

We just ran into an issue with VibeManualMemoryManagement that's as follows:

We are taking a query string parameter as found in the HTTPServerRequest.query associative array, and storing it in a table whose lifetime is global. In some cases, the value was being overwritten by the next request that reused the memory, but in others it was fine.

After a while reading through vibe's code, it was clear that the inconsistency was because formDecode() uses fresh GC memory if decoding has actually taken place. So for parameters with spaces or any sort of non-url characters, it would be fine.

I haven't read through all the code, but I suspect anything that comes out of a HTTPServerRequest cannot be trusted to have a lifetime longer than the request itself, which means appending .dup() to anything that will get stored.

This is problematic, because for example, in the case where formDecode() uses the appender, .dup() would be redundant and a pessimization.

For now, I removed VibeManualMemoryManagement from our dub.json, and will monitor GC activity to see if we can live with it for the time being.

Does it make sense?

Should we do anything to improve the situation? I think at least mentioning it in the documentation could help a bit.

I was wondering if instead of using appenders for the decoding, etc..., we could just allocate a little extra memory per request, and do any decodings in there until it's full. This way, the limitation is clear, and no memory is safe past the request's life-time.
To enforce it, we could set the memory to 0x00 or 0xdd at the end of the request, in debug builds.
Everyone who choses to use VibeManualMemoryManagement would have to understand this limitation, but the good thing is that any .dup's won't be redundant.

Any other ideas?

The long-term goal is to make VibeManualMemoryManagement the default.
The way this is supposed to work safely is to change the preferred
request handler to accept the request/response parameters as scope*.
Of course that in turn requires that scope will have been implemented
correctly. Any code that doesn't use scope parameters will
automatically get .duped data to keep everything safe.

Places that need dynamic allocation (like the mentioned appender use
in formDecode) will all use a custom pool allocator with a memory pool
that is bound to the lifetime of the request, so that basically all
request processing can be made @nogc.

It's a little sad that this will require a lot of code to be touched
when performance is a priority, because D doesn't default to scope,
which IMO would make a lot more sense. But the performance impact for
code that doesn't use scope parameters shouldn't be that big, so it
can be done gradually as needed.

BTW, the performance impact of the current VibeManualMemoryManagement
switch is quickly diminished if there are any regular GC allocations. So
using the web or REST interfaces for example will probably shadow any
performance gains of it. Going without it, but still trying to minimize
GC allocations is definitely the recommended way for complex applications.

* Or @noescape or however it will be named, assuming that it will
exist at all

Thanks for taking the time to write this up Sönke!

We've been running without VibeManualMemoryManagement for a while now with no problems, except our server apps restart about 2-3 times a day when they become unresponsive due to GC runs.

Evidently, it seems like the GC is taking more than 15 secs to run, which I guess is to be expected in this case. We've always tried as best we could to reduce unnecessary garbage generation in our user code since the beginning, but only when doing so is not too inconvenient. We've developed our own mysql driver with the main goal of generating as little garbage as possible to ease the pressure on the GC. This has worked well so far.

Before we switched, the servers had uptimes of over 3 days, which was enough, because we deploy almost every day and manually restart the apps with less than a second of downtime.

With the switch, we have 15 secs of downtime 2/3 times a day, which is ok for now, but as our traffic grows this will very soon become unacceptable, and I suppose we'll have to get creative at that point.