Bug 26998 – Decommitted spans are added to the list of normal spans rather than the returned spans in TCMalloc_PageHeap::Delete()

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Ada Chan]

In TCMalloc_PageHeap::Delete(), the deleted span can be decommitted (memory released back to the system) in the process of merging with neighboring spans that are also decommitted.  The merged span needs to be placed in the list of returned spans (spans whose memory has been returned to the system).  Right now it's always added to the list of the normal spans which can theoretically cause thrashing.  The purpose of tracking normal and returned spans is so when we need to return a free span of a certain size, we'll pick a span whose memory is already committed before a span whose memory has been decommitted and needs to be re-committed before returning.  By placing a decommitted span in the normal span list, we break this optimization.

[Ada Chan]

Created attachment 32314 [details]
Prepend the span to the returned list if it is decommitted.

[Mark Rowe (bdash)]

Comment on attachment 32314 [details]
Prepend the span to the returned list if it is decommitted.

I think it would be clearer to write it like the following (untested) as it makes the structure more obvious than mixing #if, if and ternary operators:

#if TCMALLOC_TRACK_DECOMMITED_SPANS
  if (span->decommitted) {
      if (span->length < kMaxPages)
          DLL_Prepend(&free_[span->length].returned, span);
      else
          DLL_Prepend(&large_.returned, span);
  } else
#endif
  {
      if (span->length < kMaxPages)
          DLL_Prepend(&free_[span->length].normal, span);
      else
          DLL_Prepend(&large_.normal, span);
  }

The info in the ChangeLog after the sentence ending with "can theoretically cause thrashing." appears to be restating what has been said in the first part of the ChangeLog entry.

[Darin Adler]

Comment on attachment 32314 [details]
Prepend the span to the returned list if it is decommitted.

I noticed this comment in that function:

  // Note that the spans we merge into "span" may come out of
  // a "returned" list.  For simplicity, we move these into the
  // "normal" list of the appropriate size class.

Maybe you should change it?

r=me

[Ada Chan]

I agree with the comment about the #if.  I'll end the changelog entry after "can theoretically cause thrashing."  And yes, I'll remove the comment about always prepending to the normal list!

Thanks for the review.

[Ada Chan]

Fixed in r45566.