Make space for Int32 string type

RESOLVED FIXED in mozilla17

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: evilpie, Assigned: evilpie)

Tracking

Trunk
mozilla17
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
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.
Attachment #652137 - Flags: review?(luke)

Comment 1

5 years ago
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
Attachment #652137 - Flags: review?(luke) → review+
(Assignee)

Comment 2

5 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/98e27fcf7e33
https://hg.mozilla.org/mozilla-central/rev/98e27fcf7e33
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.