Bug 43207 - WeakGCPtr's need to be updated for movable objects
Summary: WeakGCPtr's need to be updated for movable objects
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nathan Lawrence
URL:
Keywords:
Depends on: 43269 43626
Blocks:
  Show dependency treegraph
 
Reported: 2010-07-29 13:28 PDT by Nathan Lawrence
Modified: 2010-08-06 16:42 PDT (History)
6 users (show)

See Also:


Attachments
Patch (40.68 KB, patch)
2010-07-29 13:28 PDT, Nathan Lawrence
no flags Details | Formatted Diff | Diff
Patch (40.68 KB, patch)
2010-07-29 13:49 PDT, Nathan Lawrence
oliver: review-
Details | Formatted Diff | Diff
patch (23.45 KB, patch)
2010-08-04 14:15 PDT, Nathan Lawrence
no flags Details | Formatted Diff | Diff
patch (21.41 KB, patch)
2010-08-04 14:53 PDT, Nathan Lawrence
no flags Details | Formatted Diff | Diff
patch (774 bytes, patch)
2010-08-04 16:22 PDT, Nathan Lawrence
oliver: review-
Details | Formatted Diff | Diff
patch (with patch!) (23.84 KB, patch)
2010-08-05 14:21 PDT, Nathan Lawrence
ggaren: review-
Details | Formatted Diff | Diff
patch (24.18 KB, patch)
2010-08-05 15:20 PDT, Nathan Lawrence
no flags Details | Formatted Diff | Diff
patch (26.78 KB, patch)
2010-08-05 15:31 PDT, Nathan Lawrence
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nathan Lawrence 2010-07-29 13:28:13 PDT
Created attachment 62980 [details]
Patch

WeakGCPtr's should instead of directly pointing to the GC'd object should be directed to an array of pointers that can be updated for movable objects.
Comment 1 Nathan Lawrence 2010-07-29 13:49:26 PDT
Created attachment 62986 [details]
Patch

Fixed merge conflicts.
Comment 2 WebKit Review Bot 2010-07-29 13:51:46 PDT
Attachment 62986 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
JavaScriptCore/wtf/BlockAllocator.cpp:21:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
JavaScriptCore/wtf/BlockAllocator.cpp:71:  Use 0 instead of NULL.  [readability/null] [5]
JavaScriptCore/wtf/BlockAllocator.cpp:75:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
JavaScriptCore/runtime/GCHandle.h:39:  This { should be at the end of the previous line  [whitespace/braces] [4]
JavaScriptCore/runtime/GCHandle.h:83:  This { should be at the end of the previous line  [whitespace/braces] [4]
JavaScriptCore/wtf/wince/BlockAllocatorWinCE.cpp:21:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
JavaScriptCore/wtf/wince/BlockAllocatorWinCE.cpp:24:  Alphabetical sorting problem.  [build/include_order] [4]
JavaScriptCore/wtf/wince/BlockAllocatorWinCE.cpp:25:  Alphabetical sorting problem.  [build/include_order] [4]
JavaScriptCore/wtf/wince/BlockAllocatorWinCE.cpp:44:  Use 0 instead of NULL.  [readability/null] [5]
JavaScriptCore/wtf/mac/BlockAllocatorMac.cpp:21:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
JavaScriptCore/wtf/mac/BlockAllocatorMac.cpp:24:  Alphabetical sorting problem.  [build/include_order] [4]
JavaScriptCore/runtime/Collector.cpp:919:  One line control clauses should not use braces.  [whitespace/braces] [4]
JavaScriptCore/runtime/WeakGCPtr.h:53:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
JavaScriptCore/runtime/WeakGCPtr.h:61:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
JavaScriptCore/runtime/GCHandle.cpp:27:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
JavaScriptCore/runtime/GCHandle.cpp:57:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
JavaScriptCore/wtf/win/BlockAllocatorWin.cpp:21:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
JavaScriptCore/wtf/win/BlockAllocatorWin.cpp:24:  Alphabetical sorting problem.  [build/include_order] [4]
JavaScriptCore/wtf/win/BlockAllocatorWin.cpp:25:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 19 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Oliver Hunt 2010-07-29 14:02:09 PDT
Comment on attachment 62986 [details]
Patch

The BlockAllocator stuff should be drop -- you should instead add the required APIs to the new PageAllocation class that gavin added a few days ago -- you may want to discuss details with him.
Comment 4 Nathan Lawrence 2010-08-04 14:15:31 PDT
Created attachment 63491 [details]
patch
Comment 5 WebKit Review Bot 2010-08-04 14:16:17 PDT
Attachment 63491 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
JavaScriptCore/runtime/GCHandle.h:39:  This { should be at the end of the previous line  [whitespace/braces] [4]
JavaScriptCore/runtime/GCHandle.h:89:  This { should be at the end of the previous line  [whitespace/braces] [4]
JavaScriptCore/runtime/WeakGCPtr.h:53:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
JavaScriptCore/runtime/WeakGCPtr.h:61:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
JavaScriptCore/runtime/GCHandle.cpp:56:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 5 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Early Warning System Bot 2010-08-04 14:30:19 PDT
Attachment 63491 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3645322
Comment 7 Nathan Lawrence 2010-08-04 14:53:23 PDT
Created attachment 63495 [details]
patch
Comment 8 Nathan Lawrence 2010-08-04 16:22:55 PDT
Created attachment 63509 [details]
patch

The previous build errors may be related to the lack of forwarding headers in the patch.
Comment 9 WebKit Review Bot 2010-08-04 16:25:37 PDT
Attachment 63509 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
Total errors found: 0 in 0 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 Nathan Lawrence 2010-08-05 14:21:41 PDT
Created attachment 63635 [details]
patch (with patch!)
Comment 11 Geoffrey Garen 2010-08-05 14:59:50 PDT
Comment on attachment 63635 [details]
patch (with patch!)

JavaScriptCore/runtime/Collector.cpp:938
 +      m_weakGCHandlePools.append(allocation);
I think it would be better to use a smaller block allocator for WeakGCHandles, since there are usually so few of them. Maybe 32K instead of 256K?

JavaScriptCore/runtime/GCHandle.h:66
 +      }
The canonical names for these accessors in smart pointers are "get" and "set".

JavaScriptCore/runtime/GCHandle.h:78
 +      }
I would call these "nextInFreeList" and "setNextInFreeList".

JavaScriptCore/runtime/GCHandle.cpp:74
 +      return 0;
You can turn the "if" above into the ASSERT, and remove the ASSERT_NOT_REACHED and bogus return at the end.
Comment 12 Nathan Lawrence 2010-08-05 15:20:33 PDT
Created attachment 63644 [details]
patch
Comment 13 Nathan Lawrence 2010-08-05 15:31:04 PDT
Created attachment 63646 [details]
patch

This should also limit the amount of memory allocated for the weakgchandlepools.
Comment 14 Geoffrey Garen 2010-08-05 16:15:05 PDT
Comment on attachment 63646 [details]
patch

r=me
Comment 15 WebKit Commit Bot 2010-08-06 07:56:26 PDT
Comment on attachment 63646 [details]
patch

Clearing flags on attachment: 63646

Committed r64849: <http://trac.webkit.org/changeset/64849>
Comment 16 WebKit Commit Bot 2010-08-06 07:56:32 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 Adam Barth 2010-08-06 09:47:54 PDT
This patch appears to break compile on Windows.  Rolling out to verify.
Comment 18 Nathan Lawrence 2010-08-06 16:42:40 PDT
Rollout was canceled.  Thank you windows build fixer!