Last Comment Bug 783016 - Make space for Int32 string type
: Make space for Int32 string type
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: Tom Schuster [:evilpie]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: 654190
  Show dependency treegraph
 
Reported: 2012-08-15 10:34 PDT by Tom Schuster [:evilpie]
Modified: 2012-08-18 16:18 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (12.80 KB, patch)
2012-08-15 10:34 PDT, Tom Schuster [:evilpie]
luke: review+
Details | Diff | Splinter Review

Description Tom Schuster [:evilpie] 2012-08-15 10:34:31 PDT
Created attachment 652137 [details] [diff] [review]
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] 2012-08-16 09:37:10 PDT
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 3 Ryan VanderMeulen [:RyanVM] 2012-08-18 16:18:53 PDT
https://hg.mozilla.org/mozilla-central/rev/98e27fcf7e33

Note You need to log in before you can comment on or make changes to this bug.