Bug 43207 – WeakGCPtr's need to be updated for movable objects

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Nathan Lawrence]

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.

[Nathan Lawrence]

Created attachment 62986 [details]
Patch

Fixed merge conflicts.

[WebKit Review Bot]

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.

[Oliver Hunt]

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.

[Nathan Lawrence]

Created attachment 63491 [details]
patch

[WebKit Review Bot]

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.

[Early Warning System Bot]

Attachment 63491 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3645322

[Nathan Lawrence]

Created attachment 63495 [details]
patch

[Nathan Lawrence]

Created attachment 63509 [details]
patch

The previous build errors may be related to the lack of forwarding headers in the patch.

[WebKit Review Bot]

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.

[Nathan Lawrence]

Created attachment 63635 [details]
patch (with patch!)

[Geoffrey Garen]

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.

[Nathan Lawrence]

Created attachment 63644 [details]
patch

[Nathan Lawrence]

Created attachment 63646 [details]
patch

This should also limit the amount of memory allocated for the weakgchandlepools.

[Geoffrey Garen]

Comment on attachment 63646 [details]
patch

r=me

[WebKit Commit Bot]

Comment on attachment 63646 [details]
patch

Clearing flags on attachment: 63646

Committed r64849: <http://trac.webkit.org/changeset/64849>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Barth]

This patch appears to break compile on Windows.  Rolling out to verify.

[Nathan Lawrence]

Rollout was canceled.  Thank you windows build fixer!