[JSC] Memory allocation failed by fastmalloc even if heap grows successfully

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Hojong Han]

Heap tries to grow for memory allocation if there were not sufficient memory.

At first the size, more than 256 pages, is requested to the system in order to grow the heap,
but the size, less than 256 pages which can contain the demanded, is requested if it fails.

Spans more than 256 pages are managed in the large list,
and spans less than 256 pages are managed in the normal list,
so that it's wrong to get the span only from the large list after the heap grows.

[Hojong Han]

Created attachment 129131 [details]
Patch

[WebKit Review Bot]

Attachment 129131 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/JavaScriptCore/wtf/FastMalloc.cpp:1736:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1737:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1737:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1737:  Use 0 instead of NULL.  [readability/null] [5]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1740:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1747:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1753:  One space before end of line comments  [whitespace/comments] [5]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1757:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1759:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1759:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 10 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Hojong Han]

(In reply to comment #2)
> If any of these errors are false positives, please file a bug against check-webkit-style.

This entire file is in 2-space indenting. If it should be changed we should change the whole file at once.

[Geoffrey Garen]

Comment on attachment 129131 [details]
Patch

I don't think it's a good idea to duplicate this code. Instead, after calling GrowHeap(), I think it would be better to call TCMalloc_PageHeap::New recursively. This is guaranteed to terminate after one level of recursion.

[Hojong Han]

Created attachment 130742 [details]
Patch

[WebKit Review Bot]

Attachment 130742 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/JavaScriptCore/wtf/FastMalloc.cpp:1692:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1692:  grown_heap is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1732:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1733:  Use 0 instead of NULL.  [readability/null] [5]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1742:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1742:  alloc_span is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 6 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Hojong Han]

(In reply to comment #4)
> (From update of attachment 129131 [details])
> I don't think it's a good idea to duplicate this code. Instead, after calling GrowHeap(), I think it would be better to call TCMalloc_PageHeap::New recursively. This is guaranteed to terminate after one level of recursion.

I substituted the duplicated with goto sentence. 
I think so that it'd be better to call New recursively than the duplicated,
but in this case it looks simpler to use goto for recursive call only once.
Please let me know if there're more considerations.

[Geoffrey Garen]

> I substituted the duplicated with goto sentence. 

"goto" turns this code into a loop that usually executes only once. We've seen in the past that GCC and LLVM generate poor code for loops that execute only once because they assume that loops will execute more than once. I think a recursive call would be better.

[Geoffrey Garen]

> 1732  if (grown_heap)
> 1733    return NULL;

The case can never happen. If you've just grown the heap to accommodate your allocation, allocation will always succeed. I think you should remove this test.

[Hojong Han]

Created attachment 131497 [details]
Patch

[WebKit Review Bot]

Attachment 131497 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/JavaScriptCore/wtf/FastMalloc.cpp:1736:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Hojong Han]

(In reply to comment #9)
> > 1732  if (grown_heap)
> > 1733    return NULL;
> 
> The case can never happen. If you've just grown the heap to accommodate your allocation, allocation will always succeed. I think you should remove this test.
My way of thinking got stuck. 
Your comment showed a broader way. Thanks.

[Geoffrey Garen]

Comment on attachment 131497 [details]
Patch

r=me

[WebKit Review Bot]

Comment on attachment 131497 [details]
Patch

Clearing flags on attachment: 131497

Committed r110805: <http://trac.webkit.org/changeset/110805>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.