Are concatenated Delphi strings held in a hidden temporary variable that retains a reference to the string?

Question

I'm trying to understand memory issues in a Delphi server application: originally I suspected an outright leak, but now believe we're seeing memory hanging around longer than it should due to the compiler's use of a hidden temporary when dynamically concatenating strings with +, causing painful free-space memory fragmentation.

Background:

This is a suite of 32-bit server applications on Windows, Delphi version is quite old, I think it's 7 but is for sure pre-Unicode, and uses the Nexus 3 memory manager where I've written a DLL to hook all the allocate/free calls (and gigabytes of memory traces).

I have application source code but not the compiler; I am not the developer of this app (or even a Delphi dev) but have created extensive custom tools to monitor, trace, and analyze memory. I've been picking the .EXE apart in the IDA Pro disassembler.

Some sample code:

I've tried to whittle this down to the bare minimum case; this code is not intended to compile:

procedure TaskThread.RunWorkLoop
begin
    while not Terminated do
    begin

      tsk := WaitForWorkToDo();  // this could sit for minutes at a time

      SetThreadName('Working on ' + tsk.Name);

      tsk.Run(); // THIS COULD TAKE A LONG TIME

      SetThreadName('Idle');
   end
end;

SetThreadName() takes a const string parameter and hangs onto it so that other parts of the system know what this thread is doing.

My disassembly of the code shows that the compiler has allocated a hidden local temporary variable to receive the concatenation of the "Working on" and task name parts, and this is what's passed to SetThreadName, where it also retains a handle to the string.

While the task is running - and this could be 20 minutes - I believe there are two handles to the string. One is held within SetThreadName, the other is in the hidden temporary.

This is all fine and good.

Then, when the task is over and the thread name is set to 'Idle', SetThreadName() releases the original string and assigns the literal Idle.

BUT: I believe the hidden local temporary still retains a handle to that string, with a refcount=1, so it's going to take up space until either the procedure returns, or the next loop comes around to overwrite that hidden local temporary, releasing the old value.

And during this time, it's not accessible to the program, can't be explicitly released, and is serving no useful purpose but is still consuming memory.

For most procedures this doesn't matter because they start and finish relatively close to each other, so everything is released all at once, but in a looping server app, these can hang around much longer. This is causing us memory fragmentation.

It gets worse

In the actual application, it's more along the lines of:

SetThreadName(tsk.Name + '-' + FormatDateTime('mm/dd/yy hh:nn:ss', Now));

In this case, there are two hidden temporaries: one for the result of FormatDateTime, and the other for the overall concatenation result, in effect running as:

tmp1: String;
tmp2: String;
...
  tmp1 := FormatDateTime('...');
  tmp2 := tsk.Name + '-' + tmp1;
  SetThreadName(tmp2);

I am certain I'm seeing the string result of FormatDateTime hanging around in memory long after the task has completed, and I've seen it literally be a single ~30-byte allocation sitting in the middle of a 1 megabyte memory section, surrounded by free space; Nexus3MM uses VirtualAlloc to allocate larger OS-level chunks.

That single 30-byte string will be released eventually, either on the next loop or when the procedure exits, so I'm certain it's not a leak, but I would rather that single 30-byte allocation sitting in the middle of a lonely one megabyte section actually go away when we're done with it so the whole section could be released to the OS.

But if it sticks around long enough, the memory manager is going to allocate something else from it, and this hole in memory gets more permanent.

We have very detailed busy/free memory maps and are sure that this fragmentation is killing us (this is certainly not the only cause).

My Questions:

1) Am I understanding this correctly?

2) If so, is the only workaround to elide the hidden temporaries by using explicit ones, where we do things like:

tmp1: String;
tmp2: String;
...
  tmp1 := FormatDateTime('...');
  tmp2 := tsk.Name + '-' + tmp1;
  SetThreadName(tmp2);
  tmp1 := '';  // release the date/time string
  tmp2 := '';  // release the overall thread name string

I'm pretty confident I have to do this with the FormatDateTime intermediate result (I've seen it specifically), but am not sure about the overall concatenation.

This just feels wrong.

EDIT: Just an update a few weeks later. We've rewritten the central loop to use explicit temporaries, and it's actually made a noticeable (though not major) difference in memory fragmentation of some key server processes. We still have other things to look into, but it's clear to me that this was a road worth going down.

Answer 1

From my experience, it does work exactly like that. I'm not sure if this is by contract or by implementation. I guess with the recent addition of inline variable declaration, this might be slightly different now. But in pre-unicode Delphi, I believe it works exactly as you described.

All routines using variables (implicit or explicit) of a managed type, or a record containing one, will generate an implicit try/finally block in the routine, with the finally part clearing the reference. What your code really does is :

procedure TaskThread.RunWorkLoop
var
  sImplicit : string;
begin
  sImplicit := '';
  try
    while not Terminated do
    begin
      tsk := WaitForWorkToDo();  // this could sit for minutes at a time

      sImplicit := 'Working on ' + tsk.Name;

      SetThreadName(sImplicit);

      tsk.Run(); // THIS COULD TAKE A LONG TIME

      SetThreadName('Idle');
    end;
  finally
    sImplicit := '';
  end;
end;

In your situation, since you never exit the routine where the implicit variable is used, it does remain in memory.

As for a solution, I believe what you propose would work. But you could also simply move the code to another method (or a local procedure).

procedure TaskThread.RunWorkLoop
  procedure JustKeepWorking;
  begin
    tsk := WaitForWorkToDo();  // this could sit for minutes at a time
    SetThreadName('Working on ' + tsk.Name);
    tsk.Run(); // THIS COULD TAKE A LONG TIME
    SetThreadName('Idle');
  end;
begin
  while not Terminated do
  begin
    JustKeepWorking;
  end
end;

Also, you might want to check this question for additional insight.