Bug 26998 - Decommitted spans are added to the list of normal spans rather than the returned spans in TCMalloc_PageHeap::Delete()
: Decommitted spans are added to the list of normal spans rather than the retur...
Status: RESOLVED FIXED
: WebKit
New Bugs
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2009-07-06 13:30 PST by
Modified: 2009-07-06 14:14 PST (History)


Attachments
Prepend the span to the returned list if it is decommitted. (2.12 KB, patch)
2009-07-06 13:41 PST, Ada Chan
darin: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-07-06 13:30:42 PST
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.
------- Comment #1 From 2009-07-06 13:41:17 PST -------
Created an attachment (id=32314) [details]
Prepend the span to the returned list if it is decommitted.
------- Comment #2 From 2009-07-06 13:49:24 PST -------
(From update of attachment 32314 [details])
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.
------- Comment #3 From 2009-07-06 13:53:13 PST -------
(From update of attachment 32314 [details])
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
------- Comment #4 From 2009-07-06 13:54:44 PST -------
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.
------- Comment #5 From 2009-07-06 14:14:30 PST -------
Fixed in r45566.