Bug 149435

Summary: GCThreadSharedData is just a bad way of saying Heap
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: Filip Pizlo <fpizlo>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, benjamin, commit-queue, ggaren, mark.lam, mhahnenb, msaboff, nrotem, oliver, saam, sam
Priority: P2    
Version: Other   
Hardware: All   
OS: All   
Bug Depends on: 149436, 149472    
Bug Blocks: 149433    
Attachments:
Description Flags
it begins
none
the patch
mark.lam: review+
patch for landing none

Description Filip Pizlo 2015-09-21 16:35:47 PDT
The GCThreadSharedData object maps 1-to-1 to a Heap. Its existence adds nothing useful.
Comment 1 Mark Lam 2015-09-21 17:24:03 PDT
(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).
Comment 2 Filip Pizlo 2015-09-21 17:34:50 PDT
(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.
Comment 3 Filip Pizlo 2015-09-22 15:23:05 PDT
Created attachment 261769 [details]
it begins
Comment 4 Filip Pizlo 2015-09-22 16:26:04 PDT
Created attachment 261775 [details]
the patch
Comment 5 WebKit Commit Bot 2015-09-22 16:28:57 PDT
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 6 Mark Lam 2015-09-22 16:43:25 PDT
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?
Comment 7 Filip Pizlo 2015-09-22 16:45:02 PDT
(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?
Comment 8 Mark Lam 2015-09-22 16:46:25 PDT
(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 9 Mark Lam 2015-09-22 16:54:46 PDT
Comment on attachment 261775 [details]
the patch

r=me with builds fixed.
Comment 10 Filip Pizlo 2015-09-22 16:57:09 PDT
Created attachment 261781 [details]
patch for landing

Hopefully fixes the build
Comment 11 WebKit Commit Bot 2015-09-22 16:58:31 PDT
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.
Comment 12 Filip Pizlo 2015-09-22 18:36:33 PDT
Landed in http://trac.webkit.org/changeset/190151