Bug 79614 - [JSC] Memory allocation failed by fastmalloc even if heap grows successfully
Summary: [JSC] Memory allocation failed by fastmalloc even if heap grows successfully
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-26 18:43 PST by Hojong Han
Modified: 2012-03-14 18:31 PDT (History)
8 users (show)

See Also:


Attachments
Patch (2.18 KB, patch)
2012-02-27 16:49 PST, Hojong Han
no flags Details | Formatted Diff | Diff
Patch (1.83 KB, patch)
2012-03-07 18:00 PST, Hojong Han
no flags Details | Formatted Diff | Diff
Patch (1.32 KB, patch)
2012-03-12 19:49 PDT, Hojong Han
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hojong Han 2012-02-26 18:43:33 PST
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.
Comment 1 Hojong Han 2012-02-27 16:49:26 PST
Created attachment 129131 [details]
Patch
Comment 2 WebKit Review Bot 2012-02-27 16:53:53 PST
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.
Comment 3 Hojong Han 2012-02-27 17:08:03 PST
(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.
Comment 4 Geoffrey Garen 2012-03-07 11:42:03 PST
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.
Comment 5 Hojong Han 2012-03-07 18:00:53 PST
Created attachment 130742 [details]
Patch
Comment 6 WebKit Review Bot 2012-03-07 18:04:09 PST
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.
Comment 7 Hojong Han 2012-03-07 18:10:52 PST
(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.
Comment 8 Geoffrey Garen 2012-03-12 11:52:42 PDT
> 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.
Comment 9 Geoffrey Garen 2012-03-12 11:53:53 PDT
> 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.
Comment 10 Hojong Han 2012-03-12 19:49:57 PDT
Created attachment 131497 [details]
Patch
Comment 11 WebKit Review Bot 2012-03-12 19:51:44 PDT
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.
Comment 12 Hojong Han 2012-03-12 23:55:44 PDT
(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.
Comment 13 Geoffrey Garen 2012-03-14 16:25:10 PDT
Comment on attachment 131497 [details]
Patch

r=me
Comment 14 WebKit Review Bot 2012-03-14 18:30:56 PDT
Comment on attachment 131497 [details]
Patch

Clearing flags on attachment: 131497

Committed r110805: <http://trac.webkit.org/changeset/110805>
Comment 15 WebKit Review Bot 2012-03-14 18:31:01 PDT
All reviewed patches have been landed.  Closing bug.