WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
149435
GCThreadSharedData is just a bad way of saying Heap
https://bugs.webkit.org/show_bug.cgi?id=149435
Summary
GCThreadSharedData is just a bad way of saying Heap
Filip Pizlo
Reported
2015-09-21 16:35:47 PDT
The GCThreadSharedData object maps 1-to-1 to a Heap. Its existence adds nothing useful.
Attachments
it begins
(25.84 KB, patch)
2015-09-22 15:23 PDT
,
Filip Pizlo
no flags
Details
Formatted Diff
Diff
the patch
(54.03 KB, patch)
2015-09-22 16:26 PDT
,
Filip Pizlo
mark.lam
: review+
Details
Formatted Diff
Diff
patch for landing
(55.32 KB, patch)
2015-09-22 16:57 PDT
,
Filip Pizlo
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Mark Lam
Comment 1
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).
Filip Pizlo
Comment 2
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.
Filip Pizlo
Comment 3
2015-09-22 15:23:05 PDT
Created
attachment 261769
[details]
it begins
Filip Pizlo
Comment 4
2015-09-22 16:26:04 PDT
Created
attachment 261775
[details]
the patch
WebKit Commit Bot
Comment 5
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.
Mark Lam
Comment 6
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?
Filip Pizlo
Comment 7
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?
Mark Lam
Comment 8
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.
Mark Lam
Comment 9
2015-09-22 16:54:46 PDT
Comment on
attachment 261775
[details]
the patch r=me with builds fixed.
Filip Pizlo
Comment 10
2015-09-22 16:57:09 PDT
Created
attachment 261781
[details]
patch for landing Hopefully fixes the build
WebKit Commit Bot
Comment 11
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.
Filip Pizlo
Comment 12
2015-09-22 18:36:33 PDT
Landed in
http://trac.webkit.org/changeset/190151
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug