391290 - Storing GCX_MUTABLE_STRING inside JSString

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Igor Bukanov]

Currently SpiderMonkey uses GC flags associated with JSString instance to store a mutable status of the string. This requires to call js_GetGCStringFlags from various string manipulation functions. Given that js_GetGCStringFlags has to access memory that may be more than a CPU page away from the string, calling the function is expensive when the flag byte is not in CPU cache.

Thus I suggest to replace GCX_MUTABLE_STRING via storing the mutability flag inside the string itself.

Comment 1

[Igor Bukanov]

Attachment: v0.5

Here is a quilt patch as a backup of something that passes the test suite.

Comment 2

[Brendan Eich [:brendan]]

Did you see the patch for bug 157334 before filing this? Hate to duplicate effort.

/be

Comment 3

[Igor Bukanov]

(In reply to comment #2)
> Did you see the patch for bug 157334 before filing this? Hate to duplicate
> effort.

No, I have not. I was searching for GCF_LOCK a week ago, but none of the bugs were mentioned it.

Comment 4

[Igor Bukanov]

The biggest difference between the patch from comment 1 and one from bug 157334 is that the former does not add a new flag to JSString but rather observes that dependable string is always mutable in SM. This comes from JS_UndependString/JS_GetChars API that can be called at arbitrary moment to make sure that JSString.chars has 0 at JSString.chars[length]. Thus there are 4 types of strings, dependable, prefixed, growable and ordinary ones and 2 flag bits is enough to cover all cases.

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 275706 [details] [diff] [review]
v0.5

Good point, this is much better -- but a few nits:

* s/IMMUNE/IMMUTABLE/g
* JSCharBuffer is deadwood

I'll review a new patch tomorrow and adjust bug 157334 accordingly. Thanks!

/be

Comment 6

[Igor Bukanov]

Attachment: v0.6

The new version fixes the naming issue from the above comments. 

The patch requires the patch from bug 386265 comment 17 to be applicable.

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 275826 [details] [diff] [review]
v0.6


> JS_PUBLIC_API(jschar *)
> JS_GetStringChars(JSString *str)
> {
>     size_t n, size;
>-    jschar *s;
>+    jschar *chars;

Nit: this breaks the s/n vs chars/length naming convention, seems like an unnecessary change.

>+struct JSString {
>+    size_t          lengthBits;

Slight preference on my part for calling this field length, to make the normal case easier to deal with in gdb. Also gives symmetry below in macro param names (chars_, length_).

>+typedef struct JSCharArray {
>+    size_t          length;
>+    jschar          *chars;
>+} JSCharArray;

Sorry, didn't mean to cause a rename here -- find as you type was failing me for some reason, and I thought this type was unused (I hadn't read the full patch). JSCharBuffer is fine, and the static function name that still uses Buffer not Array in its name makes more sense.

Otherwise this looks great, but needs a fixed patch for bug 386265. I'll stamp it when that's updated and everything passes standalone and browser tests.

/be

Comment 8

[Igor Bukanov]

Attachment: v 0.9

Here is an updated patch to address the nits, add comments and renames into JSSTRFLAG_GROWABLE into JSSTRFLAG_MUTABLE to emphasis that with the flag set the string can be mutatted into a dependent one. This is always safe as the operation preserves the length. But growing the string is only safe as long as there is no other references on in the string.

Comment 9

[Igor Bukanov]

(In reply to comment #8)
> Created an attachment (id=275958) [details]
> v 0.9

The patch is on top of the patch from bug 386265 comment 32.

Comment 10

[Igor Bukanov]

Attachment: v1

Here comes a normal CVS diff of the last patch.

Comment 11

[Brendan Eich [:brendan]]

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

Looks good, naming and comment nits only:

>+static JSBool
>+TransferBufferToString(JSContext *cx, JSCharBuffer *buf, jsval *rval)

Canonical "buf" name refers to char[N] typed variables or related char * parameters. Suggest cb in the spirit of other names around these parts.

>+ * The GC-thing "string" type.
>+ *
>+ * When JSSTRFLAG_DEPENDENT bit of JSString.length is unset, this is a flat

"the" after "When"

could lose "JSString." and just say "length" -- up to you

"chars points to" instead of "this is"

>+ * character string owned by its GC-thing descriptor. The JString.u.chars

Ditto "JSString."

>+ * When JSSTRFLAG_DEPENDENT bit is unset, the string depends on characters of

s/bit is unset/is set/ (no need for "bit" -- if you want it, then add "the" after "When" as above)

>+ * another string accessible through JSString.u.base. The base member may

s/accessible/strongly referenced by/

>+ * point to another dependent string if JSSTRING_CHARS has not been called
>+ * yet. The length chars in a dependent string are stored starting at
>+ * base->chars + start, and are not necessarily zero-terminated.  If start is

s/base->chars/JSSTRING_CHARS(base)/

start is not defined as a bit-field within length -- worth commenting instead of just introducing it, or else use JSSTRDEP_START(str) (but that requires introducing "str")

>+ * 0, it is not stored, length is a full size_t (minus the JSSTRFLAG_* bits in
>+ * the high two positions), and the JSSTRFLAG_PREFIX flag is set.
>+ *
>+ * A flat string with JSSTRFLAG_MUTABLE bit set means the string is only

s/bit//

flat means independent here -- suggest using that as term of art, since "flat" usually just means contiguous buffer memory, not necessarily backstopped. Or define flat as independent -- it's shorter -- but do define it early in this major comment.

>+ * accessible from one thread and it is possible to turn it into a dependant

transpose "only" and "accessible"

"dependent" misspelled

>+ * string of the same length to optimize js_ConcatStrings. It also possible
>+ * to grow such string but the extreme care must be taken to ensure that no

s/the extreme/extreme/

>+ * other code relies on the original length of the string.
>  *
>  * NB: Always use the JSSTRING_LENGTH and JSSTRING_CHARS accessor macros,
>  * unless you guard str->member uses with !JSSTRING_IS_DEPENDENT(str).

This is no longer true -- just lose the ", unless ..." bug-bait.

>+ * Definitions for flags stored in the high order bits of JSString.length.
>+ * JSSTRFLAG_PREFIX and JSSTRFLAG_MUTABLE are 2 aliases for the same value.

s/2/two/

I went fast again over certain changes that seem mechanically correct. The testsuite and browser tests should cover the methods that were changed (I hope -- we'll find out).

/be

Comment 12

[Igor Bukanov]

Attachment: v1b

The new version address the nits.

Comment 13

[Igor Bukanov]

I will wait with checking in this until the bug 386265 is addressed so it is straightforward to take out the patch for bug 386265.

Comment 14

[Igor Bukanov]

(In reply to comment #13)
> I will wait with checking in this until the bug 386265 is addressed so it is
> straightforward to take out the patch for bug 386265.

I meant waiting until bug 392049 is addressed.

Comment 15

[Igor Bukanov]

Recording the patch dependancy.

Comment 16

[Igor Bukanov]

I checked in the patch from comment 12 to the trunk:

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1187245320&maxdate=1187245382&who=igor%mir2.org