445705 - SM: eliminate Namespace and QName GC things

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Igor Bukanov]

Currently Namespace and QName are implemented as separated GC things that gain an JSObject when exposed to a script. But this separated object complicates code due to the need to manage the pair (gc_thing object) instead of single entity.

Thus, to simplify code, I suggest to use JSObject for Namespace and QName and store all the necessary fields from Namespace and QName in the fixed slots of the object. In a common case when there are only few namespaces that are accessed from a script, this will in fact save memory avoiding allocating a separated GC thing.

Comment 1

[Igor Bukanov]

Attachment: v1

The patch merges JSXML(Namespace|QName) with their objects and uses the fixed slots in JSObject to store the relevant fields.

In the patch I added Get(Localname|Prefix|Url) utility methods to access the fields, but for the assignments I use explicit operations on fixed slots to avoid jsval unwrapping/wrapping.

Comment 2

[Brian Crowder]

At first blush, this looks great.  It's a big patch though, and I haven't (yet) reviewed very closely.  Is it a prerequisite to other work?

Comment 3

[Igor Bukanov]

(In reply to comment #2)
> Is it a prerequisite to other work?

Yes: at some point I would like to address 402614. To do that efficiently it would be nice to be able to allocate strings with their characters using single malloc. But that would mean that the string is no longer an ordinary GC thing and one cannot call js_GetGCThingTraceKind on it. It is possible to get rid of js_GetGCThingTraceKind, but XML-GC things complicates stuff significantly. 

Comment 4

[Igor Bukanov]

Attachment: part 1 - eliminating Namespace GC thing

To simplify the review I split the patch into 2 parts, eliminating the Namespace GC things as the first part and eliminating QName as the second. 

This is the first part, it passes shell and mochi tests on its own.

Comment 5

[Igor Bukanov]

Attachment: part 2 - eliminating QName GC thing

This is the part to eliminate Qname as GC thing.

Comment 6

[Brian Crowder]

Comment on attachment 335699 [details] [diff] [review]
v1

I'm not sure I understand why these assertions in NewXMLNamespace are interesting:

+    if (!obj)
+        return JS_FALSE;
+    JS_ASSERT(JSVAL_IS_VOID(obj->fslots[JSSLOT_PREFIX]));
+    JS_ASSERT(JSVAL_IS_VOID(obj->fslots[JSSLOT_URI]));
+    JS_ASSERT(JSVAL_IS_VOID(obj->fslots[JSSLOT_DECLARED]));

mustn't these -always- be initialized to void after the construction of the new object?


I don't think it is necessary to maintain the metering for finalizers that effectively become stub finalizers here (qname_finalize and anyname_finalize, etc.).

In other firefox code (and even in jsstr.cpp), we capitalize the acronym "URI" in identifiers, so you should probably do the same here for "GetUri" and others I might've missed.

js_EqualsStrings can assert or null-deref-crash if GetUri() and friends return NULL (which it seems they can), is that by design (it seems likely that it is, I'm just asking)?

It's nice to see XML getting some cleanup; it should act more like the other citizens of Spidermonkey-town.  But your rationale for this work is even better; I'd have reviewed yesterday, if you'd mentioned it!  :)

Only r- for now, I'll change to r+ after you finish schooling me over all my dumb questions.  ;)

Comment 7

[Igor Bukanov]

(In reply to comment #6)
> (From update of attachment 335699 [details] [diff] [review])
> I'm not sure I understand why these assertions in NewXMLNamespace are
> interesting:
> 
> +    if (!obj)
> +        return JS_FALSE;
> +    JS_ASSERT(JSVAL_IS_VOID(obj->fslots[JSSLOT_PREFIX]));
> +    JS_ASSERT(JSVAL_IS_VOID(obj->fslots[JSSLOT_URI]));
> +    JS_ASSERT(JSVAL_IS_VOID(obj->fslots[JSSLOT_DECLARED]));

This is just for self-commenting.

> I don't think it is necessary to maintain the metering for finalizers that
> effectively become stub finalizers here (qname_finalize and anyname_finalize,
> etc.).

anyname_finalize is not a stub, it clears the field in JSRuntime. But you are right about the metering code - one can get the same info using a version of CountHeap from js.cpp. So I will remove the live thing counting.

> In other firefox code (and even in jsstr.cpp), we capitalize the acronym "URI"
> in identifiers, so you should probably do the same here for "GetUri" and others
> I might've missed.

The next patch will use GetURI.

> js_EqualsStrings can assert or null-deref-crash if GetUri() and friends return
> NULL (which it seems they can), is that by design (it seems likely that it is,
> I'm just asking)?

The patch should use the samantic that the current code does. If the current code can pass two nulls to js_EqualsStrings, so should the patch.

Comment 8

[Brian Crowder]

(In reply to comment #7)
> The patch should use the samantic that the current code does. If the current
> code can pass two nulls to js_EqualsStrings, so should the patch.

Right now, js_EqualsStrings asserts that both inputs are non-null.

Comment 9

[Igor Bukanov]

(In reply to comment #8)
> (In reply to comment #7)
> > The patch should use the samantic that the current code does. If the current
> > code can pass two nulls to js_EqualsStrings, so should the patch.
> 
> Right now, js_EqualsStrings asserts that both inputs are non-null.

Yep, I forgot that. But then what is the problem? If the current code due to a hypothetical bug misses to initialize the uri field in JSXMLThing struct, then it would contain the garbage. With the patch, due to always-initialized-reserved-slots, that would correspond to GetURI returning null, which is slightly safer.

Comment 10

[Igor Bukanov]

Attachment: v2

The new version eliminates live XML GC thing metering and uses GetURI, not GetUri, as the name for the helper function.

Comment 11

[Igor Bukanov]

Attachment: part 1 - eliminating Namespace GC thing

Comment 12

[Igor Bukanov]

Attachment: part 2 - eliminating QName GC thing

Comment 13

[Igor Bukanov]

Attachment: v2 for real

The previous attachment was the wrong diff. Note that part 1 and 2 are the correct diffs.

Comment 14

[Brian Crowder]

Comment on attachment 335929 [details] [diff] [review]
v2 for real


Three cheers for working bugzilla-interdiff!

(In reply to comment #9)
> Yep, I forgot that. But then what is the problem?

As I said, I wasn't sure it was a problem, just wanted to make sure it agreed with what you'd done here.  Thanks for the explanations.

Comment 15

[Igor Bukanov]

landed - http://hg.mozilla.org/mozilla-central/rev/69d14bce003f