Bug 149435 – GCThreadSharedData is just a bad way of saying Heap

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Filip Pizlo]

The GCThreadSharedData object maps 1-to-1 to a Heap. Its existence adds nothing useful.

[Mark Lam]

(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).

[Filip Pizlo]

(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.

[Filip Pizlo]

Created attachment 261769 [details]
it begins

[Filip Pizlo]

Created attachment 261775 [details]
the patch

[WebKit Commit Bot]

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.

[Mark Lam]

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?

[Filip Pizlo]

(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?

[Mark Lam]

(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.

[Mark Lam]

Comment on attachment 261775 [details]
the patch

r=me with builds fixed.

[Filip Pizlo]

Created attachment 261781 [details]
patch for landing

Hopefully fixes the build

[WebKit Commit Bot]

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.

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/190151