Bug 22528 - Move currentThreadStackBase() to WTF
Summary: Move currentThreadStackBase() to WTF
Status: RESOLVED DUPLICATE of bug 26276
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks: 16401
  Show dependency treegraph
 
Reported: 2008-11-26 22:43 PST by Alp Toker
Modified: 2011-07-05 14:29 PDT (History)
3 users (show)

See Also:


Attachments
Share utility code for GC implementations (14.32 KB, patch)
2008-11-26 23:09 PST, Alp Toker
darin: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alp Toker 2008-11-26 22:43:04 PST
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.
Comment 1 Alp Toker 2008-11-26 23:09:39 PST
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!
Comment 2 Alexey Proskuryakov 2008-11-27 07:38:50 PST
> -static inline void* currentThreadStackBase()

I'm wondering why this function was inline.
Comment 3 Alp Toker 2008-11-28 06:42:17 PST
(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).
Comment 4 Alexey Proskuryakov 2008-11-28 08:10:14 PST
I agree, I have no idea why it was inline.
Comment 5 Darin Adler 2008-12-05 10:06:03 PST
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>.
Comment 6 Gavin Barraclough 2011-07-05 14:29:28 PDT

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