Summary: | WeakGCPtr's need to be updated for movable objects | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Nathan Lawrence <nlawrence> | ||||||||||||||||||
Component: | JavaScriptCore | Assignee: | Nathan Lawrence <nlawrence> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | abarth, commit-queue, ggaren, oliver, webkit-ews, webkit.review.bot | ||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||
OS: | All | ||||||||||||||||||||
Bug Depends on: | 43269, 43626 | ||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||
Attachments: |
|
Created attachment 62986 [details]
Patch
Fixed merge conflicts.
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 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.
Created attachment 63491 [details]
patch
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.
Attachment 63491 [details] did not build on qt: Build output: http://queues.webkit.org/results/3645322 Created attachment 63495 [details]
patch
Created attachment 63509 [details]
patch
The previous build errors may be related to the lack of forwarding headers in the patch.
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.
Created attachment 63635 [details]
patch (with patch!)
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.
Created attachment 63644 [details]
patch
Created attachment 63646 [details]
patch
This should also limit the amount of memory allocated for the weakgchandlepools.
Comment on attachment 63646 [details]
patch
r=me
Comment on attachment 63646 [details] patch Clearing flags on attachment: 63646 Committed r64849: <http://trac.webkit.org/changeset/64849> All reviewed patches have been landed. Closing bug. This patch appears to break compile on Windows. Rolling out to verify. Rollout was canceled. Thank you windows build fixer! |
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.