Closed Bug 389420 Opened 17 years ago Closed 17 years ago

ActionMonkey: Change JSGC to do MMgc-style finalization

Categories

(Core :: JavaScript Engine, defect)

Other Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jorendorff, Assigned: jorendorff)

References

Details

Attachments

(3 files, 11 obsolete files)

40.14 KB, patch
igor
: review+
Details | Diff | Splinter Review
18.81 KB, patch
Details | Diff | Splinter Review
1.80 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
JSGC does finalization through a dispatch table and the type bits in the allocation's GCFlags.  MMgc does finalization through the virtual destructor of the class MMgc::GCFinalizedObject.

Before switching to MMgc for allocation, we need to get JSGC to do MMgc-style finalization.  This involves changing all the various types of things that JSGC can finalize into C++ classes.
I think the changes are easier to understand as a series of patches.

This is the first step.  It introduces a class, JSFinalizedGCThing, which will serve as a base class for JSObject, JSString, and others.

It also adds a function, js_GetGCContext(), which finalizers can use to reach a JSContext object.  The need for this is that destructors have the same problem JS_GetStringChars() has: the caller does not provide a JSContext*.

The static function js_FinalizeGCThing(), in jsgc.cpp, will eventually replace js_FinalizeObject, js_FinalizeString, and so on as the GCFinalizeOp for all finalized GCThings.
Attachment #273613 - Flags: review?(ibukanov8)
Attached patch Part 2 - JSObject class - v1 (obsolete) — Splinter Review
Change JSObject into a subclass of JSFinalizedGCThing.  js_FinalizeObject becomes the destructor.

JS_INITIAL_SLOTS changes from 6 to 5 because JSObject::_vptr uses up some space that we were previously using for a slot.  There's a JS_STATIC_ASSERT that stops your compile if JSObject wastes any space; so it has to be either 5 or 7.
Attachment #273617 - Flags: review?(ibukanov8)
Attached patch Part 3 - JSString classes - v1 (obsolete) — Splinter Review
Same as Part 2, but for JSStrings.  This is quite a bit trickier.

jsatom.cpp is touched because the ALIGNMENT macro doesn't work when sizeof(JSString) == 12.

Per Brendan, the static asserts I removed from jsgc.cpp are obsolete (they are from 2000, regular JS_ASSERTs back then).

External string finalization is moved from jsgc.cpp to jsstr.cpp, since it's now a responsibility of the destructor ~JSExternalString().

JSString is now an abstract base class for JSNativeString (the usual string class, whose chars are allocated with JS_malloc), JSDependentString, and JSExternalString.  All the data members are in JSString, so we don't rely on "punning" anymore.

js_FinalizeString() goes away, replaced by js_FinalizeGCThing().

js/tests gives the same results with these three patches as without.
Attachment #273620 - Flags: review?(ibukanov8)
Great, will look later (Igor, feel free to look sooner; I'm on various deadlines). One thing though: making JS_INITIAL_NSLOTS 5 means function objects used by XPConnect will always need dslots, since XPConnect uses the two reserved slots. So I vote for 7 not 5. This will avoid dslots allocation for other small objects.

/be
Jason, you'll want to request review from igor@mir2.org, not ibukanov8 or any other email.

/be
Comment on attachment 273613 [details] [diff] [review]
Part 1 - abstract base class and other preliminaries - v1

Thanks, brendan.
Attachment #273613 - Flags: review?(ibukanov8) → review?(igor)
Attachment #273617 - Flags: review?(ibukanov8) → review?(igor)
Attachment #273620 - Flags: review?(ibukanov8) → review?(igor)
I will review this tomorrow, 2007-07-25.
Comment on attachment 273617 [details] [diff] [review]
Part 2 - JSObject class - v1

>-struct JSObject {
>+struct JSObject : public JSFinalizedGCThing {
>+    virtual ~JSObject();

This adds an explicit overhead of vtable meaning that it would be necessary to redo the patch once again once that overhead is optimized away. I think a better approach would be not to extend JSObject from JSFinalizedGCThing but rather add a wrapper class containing JSObject and extending from JSFinalizedGCThing. This would hide the stage0 hack from the rest of code as the only place that needs to know about the wrapper is the allocation function.
True, but what I'm trying to do is align SpiderMonkey and Tamarin so we can start replacing parts.  The approach I took is what Tamarin does.

Anything we do to JSObject will be lost when we switch to Tamarin.  The corresponding Tamarin class already has a vtable (two actually--the C++ vtable and an explicit pointer to a |VTable| object).

I guess in terms of priorities, my mission is to get Tamarin into SpiderMonkey first before thinking about potential optimizations.
(In reply to comment #9)
> Anything we do to JSObject will be lost when we switch to Tamarin.

Yes, but then IMO there is even less point to change JSObject as that will be replaced by something new. Plus IIRC strings in Tamarin are managed completely by GC so there is no need to finalize them. Thus extending JSString from something with a finalizer is a work that will be redone later. It is better to spend efforts to move string-related GC flags to the string object itself.
All right, I'll reimplement.
Hang on.

Igor, one requirement that may be missed is JS API compatibility. That means something to support JS_AddExternalStringFinalizer, etc.

Also, we are not "switching to Tamarin" in some API breaking sense, nor are we going to drop the JSObject and JSScope optimizations for dynamic objects. We may be able to lose a lot of the dynamic-uber-alles approach in SpiderMonkey, but right now, Tamarin's dynamic object compatibility and performance are not well known enough to jump.

What we need is more of a merger, especially to keep API compatibility by a process of incremental change and retesting (testsuite and Firefox at least).

Having written this, I think Jason's original design is right. We don't need to hide a stage N hack from the rest of the engine if the compiler can handle the change. Making JSObject inherit from JSFinalizedGCThing won't require pervasive source code changes AFAICT. And as Jason says, it aligns the references as well as the allocation with MMgc's not-quite-conservative scanning. That scanning code will not find an outer object from an inner one; it will only truncate the low three bits, to handle Tamarin's jsval-like tagging.

Igor, I'm not sure we can optimize away a vptr without more work on MMgc than we are looking to do. We were hoping to "just use it" (with necessary API hooks and bug fixes). Do you think a vptr per finalized thing is too much overhead?

/be
OK, I will not reimplement until I hear something more.  In the meantime I'll post patches for the XML types (JSXMLNamespace, JSXMLQName, JSXML).
Attachment #273838 - Flags: review?(igor)
Attached patch Part 5 - JSXMLQName class - v1 (obsolete) — Splinter Review
Attachment #273839 - Flags: review?(igor)
Attached patch Part 6 - JSXML class - v1 (obsolete) — Splinter Review
A slight twist here: JSXML is variable size.  Tamarin copes with this by having the various e4x objects actually be different classes; doing that in SpiderMonkey right now is possible, but it would touch almost every line of jsxml.cpp.  So instead, I added an |operator new| that lets the caller specify how many bytes to allocate.
Attachment #273845 - Flags: review?(igor)
(In reply to comment #12)
> Igor, I'm not sure we can optimize away a vptr without more work on MMgc than
> we are looking to do. We were hoping to "just use it" (with necessary API hooks
> and bug fixes). Do you think a vptr per finalized thing is too much overhead?

I do not worry about JSObject especially given JSObjectMap is effectively a vtable but I do worry about JSString. If an external strings requires a finalizer, then only it should be affected, not the rest vast majority of JSString instances.
(In reply to comment #14)
> Created an attachment (id=273838) [details]
> Part 4 - JSXMLNamespace class - v1

The finalizer for the namespace and qname is effectively empty except for metter info. That info can be recoverd if necessary at stage1 via patching MMgc. So I suggest simply remove them. The rest of stuff is OK. 

IRC log about js_IsAboutToBeFinalized:

(01:31:37 AM) ibukanov: How to deal with js_IsAboutToBeFinalized?
(01:32:13 AM) ibukanov: This is relative to the finalizer ordering and essential even for stage0.
(01:32:43 AM) jorendorff: Yep, something is already checked in for that
(01:33:07 AM) jorendorff: the JSGC implementation is just a check against the bits
(01:33:12 AM) jorendorff: something similar works for MMgc
(01:33:27 AM) Mardak|AM: https://bugzilla.mozilla.org/attachment.cgi?id=272803
(01:33:36 AM) jorendorff: However, we weren't able to make it work for non-kFinalized allocations.  It will always return false for those.
(01:33:40 AM) jorendorff: Is that ok?
(01:34:39 AM) ***jorendorff has to go.
(01:34:58 AM) jorendorff: OK, after this raft of patches, the next thing is tracing behavior.
(01:34:58 AM) ibukanov: What is presweep?
(01:35:47 AM) Mardak|AM: so potentialy JSXML wont ever IsAboutToBeFinalized if it's made to be a GCThing and not a GCFnializedThing
(01:36:13 AM) Mardak|AM: presweep is a GCCallback method that gets called after marking, before sweeping/finalizing
(01:37:01 AM) ibukanov: The problem is that JS_IsAboutToBeFinalized is a public API ans is allowed to be called for any GC thing when JSObject is finalized.
(01:38:01 AM) ibukanov: It means that it can be called even for jsdouble.
(01:39:13 AM) ibukanov: Can presweep be used to finalize JSObject?
(01:39:41 AM) Mardak|AM: what should JS_Is... return for jsdoubles and during non-GC time?
(01:40:28 AM) ibukanov: The method is only supposed to be called during finalization.
(01:41:22 AM) ibukanov: And for jsdouble* it returns the same as for any other pointer: true when the thing will be collected by GC.
(01:43:03 AM) ibukanov: Can presweep be used to call the finalizer callback for JSObject?
(01:43:59 AM) ibukanov: If so it would provide the required ordering of the finalizers.
(01:48:52 AM) Mardak|AM: sure, i dont see why not. objects can be checked if their mark bit is set and do the appropriate finalization - making sure to clear the kFinalizeFlag so that the "real" Finalize doesn't try doing it again
(01:53:55 AM) Mardak|AM: the IsAboutToBeFinalized works fine for jsdoubles that are managed by the GC.. the potential problem is calling it not during presweep and having it check an outdated mark bit
(02:08:31 AM) ibukanov: Mardak|AM: IsAboutToBeFinalized can only be called from JSObject finalization hooks or from JSGC mark_end callback. If those can be called separately from the finalization in a presweep phase, then all the problems would be solved.
Maybe we can delete MMgc::GC::IsAboutToBeFinalized() and use

  bool
  js_IsAboutToBeFinalized(void *thing)
  {
      return !MMgc::GC::GetMark(thing);
  }

This will work during lastmark() and presweep() and from finalizers.  It could return true spuriously if |thing| has actually already been swept (in a previous collection), but that's not a supported use of the API, right?
(In reply to comment #18)
> The finalizer for the namespace and qname is effectively empty except for
> metter info. That info can be recoverd if necessary at stage1 via patching
> MMgc. So I suggest simply remove them. The rest of stuff is OK. 

The JSXMLNamespace and JSXMLQName types must have a vptr anyway, since we plan to use a virtual MarkReferents() method to glue MMgc's conservative stack scanning to those objects' exact marking (bug 389713).

My plan there is to use the kFinalize bit to tell whether an object has this method or not; this forces JSXMLNamespace to share a common base class with the finalized objects, which means the finalizer will be called whether we do anything in there or not.  :-P  Do you see a better way to do it?
(In reply to comment #20)
> Maybe we can delete MMgc::GC::IsAboutToBeFinalized() and use
> 
>   bool
>   js_IsAboutToBeFinalized(void *thing)
>   {
>       return !MMgc::GC::GetMark(thing);
>   }
> 
> This will work during lastmark() and presweep() and from finalizers.  It could
> return true spuriously if |thing| has actually already been swept (in a
> previous collection), but that's not a supported use of the API, right?

I think it is also necessary to remove GCF_LOCK flag and always uses hashtable for locking in js_LockGCThing. Currently during the finalization JSGC check the bit and avoids finalizing the string if it is set. It avoids the hash entry but it also require to check for GCF_LOCK in js_IsAboutToBeFinalized.
(In reply to comment #21)
> (In reply to comment #18)
> > The finalizer for the namespace and qname is effectively empty except for
> > metter info. That info can be recoverd if necessary at stage1 via patching
> > MMgc. So I suggest simply remove them. The rest of stuff is OK. 
> 
> The JSXMLNamespace and JSXMLQName types must have a vptr anyway, since we plan
> to use a virtual MarkReferents() method to glue MMgc's conservative stack
> scanning to those objects' exact marking (bug 389713).

The issue here is to determine the dynamic type of MMgc-allocated entity to invoke the corresponding marking or finalizer method. vtable per object is just one possible approach to get the type. If it is straightforward to extend MMgc to allocate SpiderMonkey things from typed pages, then the type can be extracted from the page itself avoiding the need for vtable. Another possibility is to use some space from MMgc-alloacted page to store packed type bits.  
(In reply to comment #23)
> The issue here is to determine the dynamic type of MMgc-allocated entity to
> invoke the corresponding marking or finalizer method. vtable per object is just
> one possible approach to get the type.

Right, but I feel like we're talking across one another here.  The
higher-level task I'm working on is stage 0 - quickly getting
Spidermonkey to use MMgc for allocation.  You're proposing a new
optimization that's not currently present either in SpiderMonkey or in
MMgc.  No matter how straightforward that is, it'll take time.
Optimizing the number of bytes used per object isn't a goal.

That said, I don't think the memory usage is so bad.  Currently
SpiderMonkey uses 8 bytes per JSObject and 2 bytes per JSString on
type tags.  Tamarin uses 8 and 4, respectively.  With these patches,
**assuming GCThingFlags will go away in stage 1**, the numbers will
then be 8 and 4.  Short of perfection, admittedly.

> If it is straightforward to extend MMgc
> to allocate SpiderMonkey things from typed pages, then the type can be
> extracted from the page itself avoiding the need for vtable. Another
> possibility is to use some space from MMgc-alloacted page to store packed type
> bits.  

It doesn't seem straightforward.  I'm not sure what we'll lose by
dropping the C++ vptr.  We'll lose some debugger-friendliness, at least
in release mode-- but I think I can live with that.  I found out that if
you build MMgc with MEMORY_INFO defined, it uses RTTI (!) to generate
some friendly profiling output.  I don't know if Tamarin's JIT depends
on the layout of objects in any way.  Maybe brendan knows.  What I
*haven't* thought of worries me most of all.

The point is: I don't even have a deep understanding of how Tamarin
works yet.  Maybe you do; but for me to start trying to optimize their
code now would be premature.
(In reply to comment #24)
> That said, I don't think the memory usage is so bad.  Currently
> SpiderMonkey uses 8 bytes per JSObject and 2 bytes per JSString on
> type tags.

This is in fact 4 and 1 byte. SM uses 1 byte of flags per 2 words.

> The point is: I don't even have a deep understanding of how Tamarin
> works yet.  Maybe you do; but for me to start trying to optimize their
> code now would be premature.

Oh, I have to be clear: I am not proposing this now. vtable can be removed later as long as there is no fundamental dependencies on it. As regarding my knowledge of MMgc, then IIRC it uses OS pages to allocate things and it is rather straightforward to steal some of the page space for SM-style type flags.  
There's nothing like a vptr per gc-thing overhead in the current JSString, as Igor notes. But still, one step backward, two steps forward and all that. ;-)

/be
Attachment #274475 - Flags: review?(igor)
Attachment #274476 - Flags: review?(igor)
The only difference between this large patch and the 8 little ones is that I added |throw()| to the two |operator new| signatures that can return null; and I added |std::nothrow_t| arguments to those two signatures, which I think is the customary way of saying "by the way, this might return null" to the programmer.  (Bug 353144 discusses std::nothrow.)

You can thank js/tests for cluing me that |throw()| is necessary here.

Tests pass.
Attachment #273613 - Attachment is obsolete: true
Attachment #273617 - Attachment is obsolete: true
Attachment #273620 - Attachment is obsolete: true
Attachment #273838 - Attachment is obsolete: true
Attachment #273839 - Attachment is obsolete: true
Attachment #273845 - Attachment is obsolete: true
Attachment #274475 - Attachment is obsolete: true
Attachment #274476 - Attachment is obsolete: true
Attachment #274487 - Flags: review?(igor)
Attachment #273613 - Flags: review?(igor)
Attachment #273617 - Flags: review?(igor)
Attachment #273620 - Flags: review?(igor)
Attachment #273838 - Flags: review?(igor)
Attachment #273839 - Flags: review?(igor)
Attachment #273845 - Flags: review?(igor)
Attachment #274475 - Flags: review?(igor)
Attachment #274476 - Flags: review?(igor)
Comment on attachment 274487 [details] [diff] [review]
All changes 1-8 in one patch - v2

-struct JSDependentString {
>-    size_t          length;
>-    JSString        *base;
>+struct JSDependentString : public JSString {
>+    virtual ~JSDependentString();
>+};

I think the best thing to do at this point is to remove JSDependentString all together especially given the fact that js_UndependString effectively changes the type of the string. Otherwise the patch is r+.
Blocks: 390175
Attached patch v3Splinter Review
Does away with JSDependentString, as suggested.
Attachment #274487 - Attachment is obsolete: true
Attachment #274524 - Flags: review?(igor)
Attachment #274487 - Flags: review?(igor)
Attachment #274524 - Flags: review?(igor) → review+
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Comment on attachment 274524 [details] [diff] [review]
v3

Belated invited review -- nits, mostly:

>+static JSRuntime *
>+GetGCThingRuntime(void *thing)
>+{
>+    JSGCPageInfo *pi;
>+    JSGCArenaList *list;
>+
>+    pi = THING_TO_PAGE(thing);
>+    list = PAGE_TO_ARENA(pi)->list;
>+    list -= GC_FREELIST_INDEX(list->thingSize);
>+    return (JSRuntime *)((uint8 *)list - offsetof(JSRuntime, gcArenaList));
>+}
>+
> JSRuntime*
> js_GetGCStringRuntime(JSString *str)
> {
>-    JSGCPageInfo *pi;
>-    JSGCArenaList *list;
>-
>-    pi = THING_TO_PAGE(str);
>-    list = PAGE_TO_ARENA(pi)->list;
>-
>-    JS_ASSERT(list->thingSize == sizeof(JSGCThing));
>-    JS_ASSERT(GC_FREELIST_INDEX(sizeof(JSGCThing)) == 0);
>-
>-    return (JSRuntime *)((uint8 *)list - offsetof(JSRuntime, gcArenaList));
>+    return GetGCThingRuntime(str);

Low-cost wrapper, but tradition favors a renaming macro at most, or generalize js_GetGCStringRuntime's name, to avoid the very slight code bloat.

>+JSContext *
>+js_GetGCContext(void *thing)
>+{
>+    JSRuntime *rt = GetGCThingRuntime(thing);
>+    return rt->gcContext;

Single-use vars are discouraged except where lines get way too long, or if there's debugging value. Here I'd just write a one-liner.

>+static JSBool gc_type_has_vtable[GCX_NTYPES] = {

JSPackedBool

>+/* During garbage collection, this returns the JSContext passed to
>+ * js_GC().  |thing| must be a pointer to a thing in that runtime.
>+ *
>+ * At other times, this returns NULL.
>+ */
>+extern JSContext *
>+js_GetGCContext(void *thing);

Major comment style puts opening /* on its own line; also wraps at tw=78 (Q command in vim).

>diff -r b0b85a9e7765 js/src/jsprvtd.h
>--- a/js/src/jsprvtd.h	Mon Jul 30 13:52:06 2007 -0400
>+++ b/js/src/jsprvtd.h	Mon Jul 30 18:07:58 2007 -0400
>@@ -50,16 +50,18 @@
>  * declaring a pointer to struct type, or defining a member of struct type.
>  *
>  * A few fundamental scalar types are defined here too.  Neither the scalar
>  * nor the struct typedefs should change much, therefore the nearly-global
>  * make dependency induced by this file should not prove painful.
>  */
> 
> #include "jspubtd.h"
>+#include "MMgc.h"
>+#include <new>

Over-including here, both on first principles and in particular because this stuff should be in jsgc.h -- see below. jsprvtd.h is for typedefs (hence the td in the name -- yes, these filenames date from 8.3 hell on Windows in the '90s ;-).

>+/* Base class for all gc-managed values. */
>+class JSFinalizedGCThing : public MMgc::GCFinalizedObject {
>+public:
>+    JSFinalizedGCThing() {}
>+
>+    /*
>+     * |operator new| for JSGC.  This will soon go away, replaced by
>+     * the one for MMgc.
>+     */
>+    void *
>+    operator new(size_t nbytes, JSContext *cx, uint8 gcflags,
>+                 const std::nothrow_t &) throw();
>+
>+    /* Placement new. */
>+    void * operator new(size_t nbytes, void *place) { return place; }
>+ 
>+    /*
>+     * Custom |operator new| that allocates a buffer of a size
>+     * explicitly specified by the caller, rather than the sizeof the
>+     * object being allocated.  Used in js_NewXML().
>+     */
>+    void *
>+    operator new(size_t ignored, JSContext *cx, uint8 gcflags, size_t nbytes,
>+                 const std::nothrow_t &) throw();
>+
>+    /*
>+     * The GCThingFlags bits are currently stored by JSGC.
>+     * Once JSGC goes away, they will be stored here.
>+     */
>+    // uint8 gcflags;

Looking forward to eliminating these, or amortizing any remaining ones per page. Is that in sight yet?

>+
>+private:
>+    // Prohibit copying.
>+    JSFinalizedGCThing(const JSFinalizedGCThing &);
>+    JSFinalizedGCThing & operator=(const JSFinalizedGCThing &);
>+};

This class belongs in jsgc.h as noted above.

>+/*
>+ * String class destructors.
>+ *
>+ * Strings are unique in that the destructor is sometimes called when
>+ * GC is not happening; see js_FinalizeStringRT().  So JSString
>+ * subclass destructors must not call js_GetGCContext().
>+ */

More tw=78 wrapping whinage. Also we've stopped using "French spacing" after full stops (one space after .), thanks to Igor mainly. Final nit: I used to put () after function names, as a kind of wiki markup (this was wayyy before wikis ;-). But it's not used in SpiderMonkey comments, and it just adds noise IMHO. Try avoiding it, you'll feel pain at first (I did) but I hope find it better poor man's typography after a break-in period.

>+
>+JSNativeString::~JSNativeString()

I'm hard pressed to come up with a better name than JSNativeString. My guts says there's one out there, but it is not in sight (and possibly it's just my lunch talking ;-). So, kudos!

> {
>     JSBool valid;
> 
>+    JSRuntime *rt = js_GetGCStringRuntime(this);
>     JS_RUNTIME_UNMETER(rt, liveStrings);

Prevailing style separates decls from stmts (C vs. C++, sort of -- C++ favors interspersing decls of course, but it's still often more readable to separate even if you initialize, with one blank line).

>+static JSStringFinalizeOp str_finalizers[GCX_NTYPES] = {
>+    NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL,
>+    NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL
>+};

Why not make this half its length, biasing its defined length and all indexes into it by -GCX_EXTERNAL_STRING?

>+/*
>+ * Type for native JS strings, including ordinary strings, growable
>+ * strings, and dependent strings.
>+ *
>+ * A string with the JSSTRFLAG_DEPENDENT bit of the |length| field cleared
>+ * is an ordinary string.  |chars| points to a vector having byte size
>+ * (length + 1) * sizeof(jschar), allocated with malloc().  The buffer is
>+ * terminated at index |length| by a zero jschar.  The terminator is
>+ * purely a backstop, in case the |chars| pointer flows out to native code
>+ * that requires \u0000 termination.
>+ *
>+ * A non-dependent string is growable if the GCF_MUTABLE GCThingFlags bit
>+ * is set.  This means that a subsequent Append operation may cause this
>+ * string to change into a dependent string (the existing buffer will be
>+ * reused, but it will belong to a new string, the result of the Append).
>+ *
>+ * A dependent string depends on another string's characters.
>+ * Distinguished by the JSSTRFLAG_DEPENDENT bit being set in length.

tw=78 Q-cmd formatting wanted, plus French spacing removal. Also, the last paragraph cited here wants the first two sentences (the second's a fragment) to be joined, or otherwise revised to the second is not a fragment.

>+ * A string whose characters are not managed by SpiderMonkey.  When
>+ * this string is collected, a custom finalizer provided by the
>+ * embedder is called.  See JS_AddExternalStringFinalizer().

Last tw=78 Q2j whinage from me.

Good to see all the ActionMonkey progress!

/be
Thanks for the comments.  They're much appreciated.  I'll fix these tomorrow and post a patch for quick Mardak review.

(In reply to comment #32)
> >+    // uint8 gcflags;
>
> Looking forward to eliminating these, or amortizing any remaining ones per
> page. Is that in sight yet?

Yes, in stage 1.  We already know what we're going to do with each bit.  The stage 0 whiteboard has details.

The vptrs are harder to get rid of.
Attached patch fix nits v1 (obsolete) — Splinter Review
(In reply to comment #32)
> > JSRuntime*
> > js_GetGCStringRuntime(JSString *str)
> > {
> >+    return GetGCThingRuntime(str);
> Low-cost wrapper, but tradition favors a renaming macro at most, or generalize
> js_GetGCStringRuntime's name, to avoid the very slight code bloat.
Merged to js_GetGCThingRuntime and extern unstatic.

> >+    JSRuntime *rt = GetGCThingRuntime(thing);
> >+    return rt->gcContext;
> Single-use vars are discouraged except where lines get way too long, or if
> there's debugging value. Here I'd just write a one-liner.
Joined.

> >+static JSBool gc_type_has_vtable[GCX_NTYPES] = {
> JSPackedBool
Packed-ified.

> >+/* During garbage collection, this returns the JSContext passed to
> Major comment style puts opening /* on its own line; also wraps at tw=78 (Q
> command in vim).
I typically use "gq{motion}" w/ ":set nojs" for this.. Q goes into Ex for me..

> >+++ b/js/src/jsprvtd.h	Mon Jul 30 18:07:58 2007 -0400
> >+#include "MMgc.h"
> >+#include <new>
> Over-including here, both on first principles and in particular because this
> stuff should be in jsgc.h -- see below. jsprvtd.h is for typedefs
Moved to jsgc.h which needs jsstr.h including for JSFinalizedGCThing, but jsgc.h now needs jsapi.h for JSTRACE_STRING, etc.

> Also we've stopped using "French spacing" after full stops (one space
> after .), thanks to Igor mainly. Final nit: I used to put () after
> function names, as a kind of wiki markup (this was wayyy before wikis
Un-Frenched. De-()-ed.

> >+    JSRuntime *rt = js_GetGCStringRuntime(this);
> Prevailing style separates decls from stmts
Split.

> >+static JSStringFinalizeOp str_finalizers[GCX_NTYPES] = {
> >+    NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL,
> >+    NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL
> >+};
> Why not make this half its length, biasing its defined length and all indexes
> into it by -GCX_EXTERNAL_STRING?
Shifted with a #define that could probably be better named...
Attachment #274867 - Flags: review?(jorendorff)
(In reply to comment #34)
> > >+++ b/js/src/jsprvtd.h	Mon Jul 30 18:07:58 2007 -0400
> > >+#include "MMgc.h"
> > >+#include <new>
> > Over-including here, both on first principles and in particular because this
> > stuff should be in jsgc.h -- see below. jsprvtd.h is for typedefs
> Moved to jsgc.h which needs jsstr.h including for JSFinalizedGCThing,

JSFinalizedGCThing should be in declared in jsgc.h.

> but
> jsgc.h now needs jsapi.h for JSTRACE_STRING, etc.

JSTRACE_* should be in jspubtd.h, don't nest jsapi.h in jsgc.h.

> Shifted with a #define that could probably be better named...

How about EXTERNAL_ONLY ?

/be
(In reply to comment #35)
> JSTRACE_* should be in jspubtd.h, don't nest jsapi.h in jsgc.h.
There's some static asserts and function declarations that need..

jsapi.h:(* JS_DLL_CALLBACK JSGCRootMapFun)(void *rp, const char *name, void *data);
jsapi.h:#define JSTRACE_STRING  2
jsapi.h:#define JSVAL_NULL              OBJECT_TO_JSVAL(0)

> JSFinalizedGCThing should be in declared in jsgc.h.
Yup.

> > Shifted with a #define that could probably be better named...
> How about EXTERNAL_ONLY ?
Sounds good.
(In reply to comment #36)
> (In reply to comment #35)
> > JSTRACE_* should be in jspubtd.h, don't nest jsapi.h in jsgc.h.
> There's some static asserts and function declarations that need..
> 
> jsapi.h:(* JS_DLL_CALLBACK JSGCRootMapFun)(void *rp, const char *name, void
> *data);
> jsapi.h:#define JSTRACE_STRING  2
> jsapi.h:#define JSVAL_NULL              OBJECT_TO_JSVAL(0)

* Move function pointer typedefs including JSGCRootMapFun to jspubtd.h.

* Static assertions in jsgc.h could move to jsgc.c, I think that would be better for several reasons (header file modularity defined as not over-including is one of them). We only need one expansion of any given static assertion, not N for the N #includes of the .h file that currently includes the static assertion. So putting it in the .c that goes with that .h seems best.

/be
Comment on attachment 274867 [details] [diff] [review]
fix nits v1

r+, this is an improvement--but please also address Brendan's comments on this patch.  Also:

>+ * A dependent string depends on another string's characters. Distinguished by
>+ * the JSSTRFLAG_DEPENDENT bit being set in length. The |base| member may point

Still a fragment.  Easily fixed.  Dependent clause.
Attachment #274867 - Flags: review?(jorendorff) → review+
Attached patch fix nits v1.1 (obsolete) — Splinter Review
(In reply to comment #37)
> > jsapi.h:(* JS_DLL_CALLBACK JSGCRootMapFun)(void *rp, const char *name, void
> > *data);
> > jsapi.h:#define JSTRACE_STRING  2
> > jsapi.h:#define JSVAL_NULL              OBJECT_TO_JSVAL(0)
> * Move function pointer typedefs including JSGCRootMapFun to jspubtd.h.
Moved the #define constants as well -- the return values of the function pointer.
> * Static assertions in jsgc.h could move to jsgc.c
Moved.

(In reply to comment #38)
> >+ * A dependent string depends on another string's characters. Distinguished by
> >+ * the JSSTRFLAG_DEPENDENT bit being set in length. The |base| member may point
> Still a fragment.  Easily fixed.  Dependent clause.
Oops. Added a conjunction + linking verb.
Attachment #274867 - Attachment is obsolete: true
Comment on attachment 274975 [details] [diff] [review]
fix nits v1.1

:(  The comment for JS_MapGCRoots suffers from being divided between two different files like that.  It might be better to keep it all in jsapi.h and make the comment in jspubtd.h a cross-reference.

Apart from that -- looks fine.
Attached patch fix nits v1.2Splinter Review
Attachment #274975 - Attachment is obsolete: true
Comment on attachment 275022 [details] [diff] [review]
fix nits v1.2

Restored comments and defines to jsapi.h.

remote: added 1 changesets with 7 changes to 7 files
changeset 3740:  	4a9a42cd66a6
Flags: in-testsuite-
Fix some compilation errors for some build configs e.g., JS_THREADSAFE
Attachment #275892 - Flags: review?(jorendorff)
Attachment #275892 - Flags: review?(jorendorff) → review+
had to move the #include in jsxml because the EXTERN_C isn't moved yet on hg/actionmonkey

pushed to hg/actionmonkey
remote: added 1 changesets with 3 changes to 3 files
changeset 4357:  	852f0c39fd60
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: