Bug 26998 - Decommitted spans are added to the list of normal spans rather than the returned spans in TCMalloc_PageHeap::Delete()
Summary: Decommitted spans are added to the list of normal spans rather than the retur...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Ada Chan
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-07-06 13:30 PDT by Ada Chan
Modified: 2009-07-06 14:14 PDT (History)
0 users

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Ada Chan 2009-07-06 13:30:42 PDT
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 Ada Chan 2009-07-06 13:41:17 PDT
Created attachment 32314 [details]
Prepend the span to the returned list if it is decommitted.
Comment 2 Mark Rowe (bdash) 2009-07-06 13:49:24 PDT
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.
Comment 3 Darin Adler 2009-07-06 13:53:13 PDT
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
Comment 4 Ada Chan 2009-07-06 13:54:44 PDT
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 Ada Chan 2009-07-06 14:14:30 PDT
Fixed in r45566.