Bug 22528 – Move currentThreadStackBase() to WTF

• Component: JavaScriptCore
• Status: RESOLVED DUPLICATE

[Alp Toker]

currentThreadStackBase() is currently a static function in JavaScriptCore/runtime/Collector.cpp

The GObject DOM binding will implement a conservative stack-only GC for wrapper objects and C strings and needs a way to get the current thread stack base in the marking phase.

Since this code has delicate platform-specific conditionals and isn't JSC-specific, it's best split out and moved into WTF where it can be reused. Introducing a new header for GC utility functions will also pave the way for sharing more of the JSC collector implementation with DOM bindings and WebKit API layer code which could benefit from garbage collection.

[Alp Toker]

Created attachment 25554 [details]
Share utility code for GC implementations

        Split out JSC collector internals to WTF so the code can be shared
        with DOM bindings and WebKit API layer code implementing garbage
        collection.

        This patch splits out what's necessary to support the proposed
        conservative stack-only GC in the GObject C DOM binding.

 JavaScriptCore/AllInOneFile.cpp                     |    1 
 JavaScriptCore/ChangeLog                            |   25 ++++
 JavaScriptCore/GNUmakefile.am                       |    2 
 JavaScriptCore/JavaScriptCore.pri                   |    1 
 JavaScriptCore/JavaScriptCore.scons                 |    1 
 JavaScriptCore/JavaScriptCore.vcproj/WTF/WTF.vcproj |    8 +
 JavaScriptCore/JavaScriptCoreSources.bkl            |    1 
 JavaScriptCore/runtime/Collector.cpp                |   73 ------------
 JavaScriptCore/wtf/CollectorUtilities.cpp           |  117 ++++++++++++++++++++
 JavaScriptCore/wtf/CollectorUtilities.h             |   35 +++++
 WebCore/ChangeLog                                   |   17 ++
 WebCore/ForwardingHeaders/wtf/CollectorUtilities.h  |    1 
 WebCore/WebCore.vcproj/WebCore.vcproj               |    4 
 13 files changed, 214 insertions(+), 72 deletions(-)

Note that the patch doesn't add the new files to the XCode project files as I don't have access to the IDE right now. To the reviewer: Please update those files when landing this, or otherwise update the patch with the necessary changes so I can get them in. Thanks!

[Alexey Proskuryakov]

> -static inline void* currentThreadStackBase()

I'm wondering why this function was inline.

[Alp Toker]

(In reply to comment #2)
> > -static inline void* currentThreadStackBase()
> 
> I'm wondering why this function was inline.
> 

I added CollectorUtilities.cpp to AllInOneFile.cpp just above Collector.cpp, guessing gcc will inline it if appropriate at -O3. If you're concerned it might affect performance, could you try running a perf test with this patch applied? I don't think inlining or not will affect the correctness of the result (this function is for finding the stack base, not stack pointer so it's fine as a function call).

[Alexey Proskuryakov]

I agree, I have no idea why it was inline.

[Darin Adler]

Comment on attachment 25554 [details]
Share utility code for GC implementations

I don't like the word "utilities" in a file name; it seems to be a word without meaning. I suggest calling this something like MachineStack.h.

If the macro is going to be in a header file, then I think it should be changed into an inline function that takes a const void*, so we don't have to add a WTF_ prefix to it.

Why include <stdlib.h>? I think all you need in this header is the type intptr_t, and that's not in <stdlib.h>.

[Gavin Barraclough]

*** This bug has been marked as a duplicate of bug 26276 ***