783016 - Make space for Int32 string type

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tom S [:evilpie]]

Attachment: v1

So the basic change here is that replaced the encoding of Ropes with 0b000, so we can still test for non-linear/ropeness with two machine instructions.

I also adjusted how we test for atoms, because I am going to need some easy way to create an Int32Atom. I also added one single bit for HasBase (dependent/undepended) strings, if we ever need more bits we could change that.

Comment 1

[Luke Wagner [:luke]]

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

Review of attachment 652137 [details] [diff] [review]:
-----------------------------------------------------------------

Nicely done!  Good job shuffling the bits around :)

So, IIUC, there will be two two varieties of Int32 strings: atomized and non-atomized.  Is the idea that making a non-atomized string is much faster (no hash-lookup required) and, when we do atomize, we'd rather not convert to chars?  Will there be two hash maps: chars -> non-int-atoms, int -> int-atoms?

::: js/src/vm/String.h
@@ +178,5 @@
>       * the predicate used to query whether a JSString instance is subtype
>       * (reflexively) of that type.
>       *
> +     *   Rope         0000       0000
> +     *   Linear       -          xxxx

To make the case analysis order-independent, could you change 'xxxx' to '!0000'

@@ +179,5 @@
>       * (reflexively) of that type.
>       *
> +     *   Rope         0000       0000
> +     *   Linear       -          xxxx
> +     *   HasBase      -          xxx1

Good idea including this in the table to make the assumption clear.  Unfortunately, we now need to document "HasBase" ;-)  How about after this table you just have a comment to the effect of "'HasBase' here refers to the two string types that have a 'base' field: JSDependentString and JSUndependedString.  A JSUndependedString is a JSDependentString which has been 'fixed' (by ensureFixed) to be null-terminated.  In such cases, the string must keep marking its base since there may be any number of *other* JSDependentStrings transitively depending on it.".

@@ +352,5 @@
>      }
>  
>      JS_ALWAYS_INLINE
>      bool isAtom() const {
> +        return !!(d.lengthAndFlags & ATOM_BIT);

I'd be happy to leave out the !!.  (I think we've turned off the annoying msvc warning.)

@@ +365,5 @@
>      /* Only called by the GC for dependent or undepended strings. */
>  
>      inline bool hasBase() const {
> +        JS_STATIC_ASSERT((DEPENDENT_FLAGS | JS_BIT(1)) == UNDEPENDED_FLAGS);
> +        return !!(d.lengthAndFlags & HAS_BASE_BIT);

ditto

Comment 2

[Tom S [:evilpie]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/98e27fcf7e33

Comment 3

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/98e27fcf7e33