608778 - Rename JSString::MUTABLE to JSString::EXTENSIBLE

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: Change MUTABLE to HACKABLE.

The MUTABLE does not imply mutability of characters within the string, it implies that the string's extra storage capacity may be co-opted via a concat operation. 

This small patch simply changes the name to be less misleading and adds documentation of this optimization.

I didn't remove any of the comments on thread safety, since it may still be relevant to compartments work.

Comment 1

[Chris Leary [:cdleary] (not checking bugmail)]

IRC impetus:

<cdleary> jorendorff: can we get rid of the mutable flag on strings now? or is there some case where multiple threads want to access a single immutable string still
<jorendorff> cdleary: some strings are still shared, actually
<luke> jorendorff: do tell
<jorendorff> see AtomState
<jorendorff> luke: Actually right now all atoms are allocated in the default compartment (see AutoLockDefaultCompartment in js_AtomizeString
<jorendorff> luke: because we have uniqueness guarantees that we still aren't sure we should break
<jorendorff> i think we should, but not right now
<luke> jorendorff: ah, atoms, that makes sense.  then would |str->isAtomized() iff !str->isMutable()| hold?
<jorendorff> we could make something like that hold
<jorendorff> right now there's JS_MakeStringImmutable (which still has a very narrow possible use: it causes JS_GetStringChars to return a null-terminated string infallibly)
<jorendorff> IMHO that isn't worth keeping around
<jorendorff> there are static strings which can't be mutable either
<jorendorff> but,  str->isMutable()   <===>  str->isAtomized() || str->isStatic()
<jorendorff> sounds good to me
<jorendorff> and then get rid of the bit
<luke> jorendorff: and the confusion (which recently unfolded) about "does mutable mean the chars are mutable?"
<jorendorff> well, just renaming the silly thing would accomplish that
<jorendorff> canBeHackedByAppendString() or whatever

Comment 2

[Chris Leary [:cdleary] (not checking bugmail)]

The browser only seems to use MakeStringImmutable in:

- nsContentUtils::CloneSimpleValues: it says "to avoid copying" -- maybe this is a misunderstanding of our use of the term "mutable"?)
- XPCVariant::InitializeData to ensure null termination, which is grossly more side-effecty, but we can ensure null termination other ways

jorendorff, if you want, we can probably r- and change this bug to rip that API out instead and have an implied hackability of strings through js_ConcatStrings.

Comment 3

[Chris Leary [:cdleary] (not checking bugmail)]

Er... I mean implied hackability given that a string is not static nor atomized.

Comment 4

[Jeff Walden [:Waldo]]

I suggested JSString::EXTENDABLE in real life as possibly more precisely indicating its meaning than HACKABLE, which doesn't suggest any intrinsic functionality to me.  More thought also brings to mind APPENDING_ALLOWED, which communicates meaning exactly at cost of a few more characters.

Comment 5

[Brendan Eich [:brendan]]

Waldo's right, there's a better name that connotes realloc-able. It has to do with the address possibly changing. It's pretty subtle. Not sure you're going to find a pithy name.

In any event the API name-part should stay abstract, in case we do other kinds of mutating optimizations in the future.

/be

Comment 6

[Chris Leary [:cdleary] (not checking bugmail)]

(In reply to comment #5)
> In any event the API name-part should stay abstract, in case we do other kinds
> of mutating optimizations in the future.

Sorry, I'm a little unclear on what you're saying here -- do you object to removing the JS_MakeStringImmutable API?

IMO the fact that we might change the JSString internals shouldn't be exposed across the API boundary (as JS_MakeStringImmutable seems to do), given that the user is already promising not to break API invariants (i.e. enter a request on cx before any concat occurs, don't share strings between threads).

Comment 7

[Brendan Eich [:brendan]]

Why are you trying to remove JS_MakeStringImmutable? It is needed by our own code and it looks like it will continue to be needed:

http://mxr.mozilla.org/mozilla-central/search?string=JS_MakeStringImmutable

/be

Comment 8

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: Change MUTABLE to EXTENSIBLE.

Talked to Brendan IRL about the applications in which we still want the shared-immutability of strings to be exposed through the JSAPI.

Comment 9

[Jason Orendorff [:jorendorff]]

Comment on attachment 487685 [details] [diff] [review]
Change MUTABLE to EXTENSIBLE.

Needs a test.

Just kidding. r=me.

Comment 10

[Jason Orendorff [:jorendorff]]

I actually rather liked HACKABLE, but I'll take what I can get.

Comment 11

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/tracemonkey/rev/f50d1429af86

Comment 12

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/f50d1429af86