The GCThreadSharedData object maps 1-to-1 to a Heap. Its existence adds nothing useful.
(In reply to comment #0) > The GCThreadSharedData object maps 1-to-1 to a Heap. Its existence adds > nothing useful. I wouldn't say that it adds nothing. It does encapsulate the GC thread management code, and I think there is value in modularizing the code as opposed to having it all in one giant heap class. That said, if it is adding complexity that is preventing your concurrent GC work, I won't object to its removal (especially since it may not be the best abstraction for breaking down the heap when you're done with the concurrent GC design).
(In reply to comment #1) > (In reply to comment #0) > > The GCThreadSharedData object maps 1-to-1 to a Heap. Its existence adds > > nothing useful. > > I wouldn't say that it adds nothing. It does encapsulate the GC thread > management code, and I think there is value in modularizing the code as > opposed to having it all in one giant heap class. That said, if it is > adding complexity that is preventing your concurrent GC work, I won't object > to its removal (especially since it may not be the best abstraction for > breaking down the heap when you're done with the concurrent GC design). It's basically a struct since everyone who deals with it is a friend and accesses its members directly. So, it's a purely harmful class - it provides no encapsulation but it does mean more code for accessing its data.
Created attachment 261769 [details] it begins
Created attachment 261775 [details] the patch
Attachment 261775 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/heap/SlotVisitor.cpp:232: More than one command on the same line [whitespace/newline] [4] ERROR: Source/JavaScriptCore/heap/GCThread.cpp:72: More than one command on the same line [whitespace/newline] [4] ERROR: Source/JavaScriptCore/heap/GCThread.cpp:78: More than one command on the same line [whitespace/newline] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:372: More than one command on the same line [whitespace/newline] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:623: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:1581: More than one command on the same line [whitespace/newline] [4] Total errors found: 6 in 14 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 261775 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=261775&action=review > Source/JavaScriptCore/heap/CopyVisitor.cpp:59 > - m_shared.m_copiedSpace->recycleEvacuatedBlock(block, m_shared.m_vm->heap.operationInProgress()); > + m_heap.m_storageSpace.recycleEvacuatedBlock(block, m_heap.operationInProgress()); This is a change of behavior. Is this intentional?
(In reply to comment #6) > Comment on attachment 261775 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=261775&action=review > > > Source/JavaScriptCore/heap/CopyVisitor.cpp:59 > > - m_shared.m_copiedSpace->recycleEvacuatedBlock(block, m_shared.m_vm->heap.operationInProgress()); > > + m_heap.m_storageSpace.recycleEvacuatedBlock(block, m_heap.operationInProgress()); > > This is a change of behavior. Is this intentional? It's not meant to be. What behavior do you think will be different?
(In reply to comment #7) > (In reply to comment #6) > > Comment on attachment 261775 [details] > > the patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=261775&action=review > > > > > Source/JavaScriptCore/heap/CopyVisitor.cpp:59 > > > - m_shared.m_copiedSpace->recycleEvacuatedBlock(block, m_shared.m_vm->heap.operationInProgress()); > > > + m_heap.m_storageSpace.recycleEvacuatedBlock(block, m_heap.operationInProgress()); > > > > This is a change of behavior. Is this intentional? > > It's not meant to be. What behavior do you think will be different? Ugggh. My reading error. m_copiedSpace == m_storageSpace.
Comment on attachment 261775 [details] the patch r=me with builds fixed.
Created attachment 261781 [details] patch for landing Hopefully fixes the build
Attachment 261781 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/heap/SlotVisitor.cpp:232: More than one command on the same line [whitespace/newline] [4] ERROR: Source/JavaScriptCore/heap/GCThread.cpp:72: More than one command on the same line [whitespace/newline] [4] ERROR: Source/JavaScriptCore/heap/GCThread.cpp:78: More than one command on the same line [whitespace/newline] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:372: More than one command on the same line [whitespace/newline] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:623: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:1581: More than one command on the same line [whitespace/newline] [4] Total errors found: 6 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.
Landed in http://trac.webkit.org/changeset/190151