977117 - Inline ObjectIsTypeDescriptor() intrinsic in ion

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Niko Matsakis [:nmatsakis]]

The self-hosted code would like to cheaply test whether a given object is a type descriptor. Lars has been working on a patch in bug 976504.

Comment 1

[Lars T Hansen]

Attachment: Patch

This has been rewritten with a multi-valued map-across-classes method on TemporaryTypeSet, as discussed on bug 976504.

Comment 2

[Niko Matsakis [:nmatsakis]]

Comment on attachment 8383049 [details] [diff] [review]
Patch

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

Very nice.j

::: js/src/jsinfer.cpp
@@ +1723,5 @@
> +            if (true_results) return ForAllResult::MIXED;
> +        }
> +    }
> +
> +    return true_results ? ForAllResult::ALL_TRUE : ForAllResult::ALL_FALSE;

Maybe add JS_ASSERT((true_results || false_results) && !(true_results && false_results));

::: js/src/jsinfer.h
@@ +674,5 @@
>      const Class *getKnownClass();
>  
> +    /* Result returned from forAllClasses */
> +    enum ForAllResult {
> +        EMPTY=1,                // Set empty

Any particular reason you started this at 1?

Comment 3

[Lars T Hansen]

(In reply to Niko Matsakis [:nmatsakis] from comment #2)
> Comment on attachment 8383049 [details] [diff] [review]
> Patch
> 
> Review of attachment 8383049 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Very nice.j
> 
> ::: js/src/jsinfer.cpp
> @@ +1723,5 @@
> > +            if (true_results) return ForAllResult::MIXED;
> > +        }
> > +    }
> > +
> > +    return true_results ? ForAllResult::ALL_TRUE : ForAllResult::ALL_FALSE;
> 
> Maybe add JS_ASSERT((true_results || false_results) && !(true_results &&
> false_results));

Sure, I can do that.

> ::: js/src/jsinfer.h
> @@ +674,5 @@
> >      const Class *getKnownClass();
> >  
> > +    /* Result returned from forAllClasses */
> > +    enum ForAllResult {
> > +        EMPTY=1,                // Set empty
> 
> Any particular reason you started this at 1?

Yes, though maybe not a good reason from where you're sitting.  I'm trying to cultivate a habit where '0' is not used to carry meaning for bitfields and enums:

I got burned once because, on Unix, O_RDONLY|O_WRONLY != O_RDWR, even though it is otherwise defined that the O_whatever flags are bits that can be OR'ed together.  The underlying problem is that O_RDONLY is defined as 0.  This is a known problem that I tracked through available source code back to at least BSD 2.12 (ca 1980), at which point fcntl.h had been introduced and was providing these flags (7th Edition did not have the header or the flags).  Despite being a known bug for 35 years it is basically not fixable for binary compatibility reasons.

We're not dealing with a public API with binary compatibility issues here, but I still think it's a useful habit to cultivate...

Comment 4

[Lars T Hansen]

Attachment: Patch v2

Added the requested assert (in a slightly more compact form), and clarified the commit message in the patch.  Otherwise unchanged.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9cb5d636acdf

In the future, please just set the checkin-needed bug keyword when your patch is ready to land.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9cb5d636acdf