Summary: | Decommitted spans are added to the list of normal spans rather than the returned spans in TCMalloc_PageHeap::Delete() | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ada Chan <adachan> | ||||
Component: | New Bugs | Assignee: | Ada Chan <adachan> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | ||||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | PC | ||||||
OS: | OS X 10.5 | ||||||
Attachments: |
|
Description
Ada Chan
2009-07-06 13:30:42 PDT
Created attachment 32314 [details]
Prepend the span to the returned list if it is decommitted.
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 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
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. |