938294 - Add a meaningful way to mark DOM methods [Pure]

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

For now, just going to make pure ones have an alias set that doesn't alias pure DOM getters.  This depends on knowing that our argument conversions won't have side-effects.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Allow the [Pure] annotation on non-throwing DOM methods.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Store information about argument types in jitinfo.

Please note the comment about whether we're using the right definition of Primitive

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Make use of the [Pure] annotations on methods and analysis of their arguments to make them not look like writes to alias analysis.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Part 4 still coming up, adding a bunch of [Pure] annotations.

Comment 5

[Boris Zbarsky [:bzbarsky]]

One more note for part 3.  If that code is called a lot, maybe we want to stash the alias set value in a member...

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Add more [Pure] annotations on Node/Element/Document.

Comment 7

[Eric Faust [:efaust]]

Comment on attachment 831727 [details] [diff] [review]
part 2.  Store information about argument types in jitinfo.

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

makes sense. r=me

::: dom/bindings/Codegen.py
@@ +6058,5 @@
>  
>      def defineJitInfo(self, infoName, opName, opType, infallible, constant,
> +                      pure, hasSlot, slotIndex, returnTypes, args):
> +        """
> +        argTypes is None if we don't want to output argTypes for some

nit: "args is None if..."?

@@ +6077,5 @@
> +            argTypes = "%s_argTypes" % infoName
> +            args = [CGMemberJITInfo.getJSArgType(arg.type) for arg in args]
> +            args.append("JSJitInfo::ArgTypeListEnd")
> +            argTypesDecl = (
> +                "static const JSJitInfo::ArgType %s_argTypes[] = { %s };\n" %

can we reuse |argTypes| here? Is that better?

@@ +6091,5 @@
>                  "  %s,\n"
>                  "  JSJitInfo::%s,\n"
>                  "  %s,  /* isInfallible. False in setters. */\n"
>                  "  %s,  /* isConstant. Only relevant for getters. */\n"
> +                "  %s,  /* isPure.  Only relevant for getters and methods. */\n"

nit: "False in setters" for consistency? "Not relevant in setters"?

@@ +6096,3 @@
>                  "  %s,  /* hasSlot.  Only relevant for getters. */\n"
>                  "  %d,  /* Reserved slot index, if we're stored in a slot, else 0. */\n"
> +                "  %s,  /* returnType.  Only relevant for getters/methods. */\n"

nit: even if we don't change to "False in setters", can we at least standardize between "for getters/methods" and "for getters and methods"?

@@ +6308,5 @@
> +            # But TI treats "double" as meaning "int or double", so we're
> +            # good to return JSVAL_TYPE_DOUBLE here.
> +            return "JSJitInfo::Double"
> +        if tag != IDLType.Tags.uint32:
> +            raise TypeError("No idea what type " + str(t) + " is.")

nit: is this better than the assert used above?

::: dom/webidl/Element.webidl
@@ +36,5 @@
>    readonly attribute DOMTokenList? classList;
>  
>    [SameObject]
>    readonly attribute MozNamedAttrMap attributes;
> +  [Pure]

Are we intending to leave this, and then do another annotations pass?

::: js/src/jsfriendapi.h
@@ +1444,5 @@
> +        // Should "Primitive" use the WebIDL definition, which
> +        // excludes string and null, or the typical JS one that includes them?
> +        Primitive = Integer | Double | Boolean | Null | String,
> +        ObjectOrNull = Object | Null,
> +        Any = ObjectOrNull | Primitive,

This wants some static asserts about the state of Any with respect to the other values.

Comment 8

[Eric Faust [:efaust]]

Comment on attachment 831730 [details] [diff] [review]
part 3.  Make use of the [Pure] annotations on methods and analysis of their arguments to make them not look like writes to alias analysis.

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

Good. r=me. We should think about the additional MIR, as an aesthetic fix.

::: js/src/jit/MIR.h
@@ +1874,5 @@
>      TypePolicy *typePolicy() {
>          return this;
>      }
>      AliasSet getAliasSet() const {
> +        if (isDOMFunction()) {

Boy, this makes me want to split the DOM stuff out of MCall... It's sorta starting to look like the inmates are taking over the institution...Perhaps I should look again into what it would take to make an MDOMCall.

@@ +1884,5 @@
> +            if (jitInfo->isPure && jitInfo->argTypes) {
> +                uint32_t argIndex = 0;
> +                for (const JSJitInfo::ArgType* argType = jitInfo->argTypes;
> +                     *argType != JSJitInfo::ArgTypeListEnd;
> +                     ++argType, ++argIndex) {

nit: multi-line for has brace on next line.

@@ +1897,5 @@
> +                    // something that might be an object to an argument that
> +                    // expects a numeric, string, or boolean value.
> +                    if ((actualType == MIRType_Value || actualType == MIRType_Object) &&
> +                        (*argType &
> +                         (JSJitInfo::Boolean | JSJitInfo::String | JSJitInfo::Numeric))) {

nit: multi-line if conditions have brace on next line.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> nit: "args is None if..."?

Yes.

> can we reuse |argTypes| here? Is that better?

Yeah.  Done.

> nit: "False in setters" for consistency? "Not relevant in setters"?

I can do "Not relevant for setters".

> can we at least standardize between "for getters/methods" and "for getters and methods"?

I just changed this to "Not relevant for setters" as well, but yeah, I think one of my followup patches in bug 938355 had standardized.

> nit: is this better than the assert used above?

It's more informative than the asserts; unlike the asserts this could actually get hit if someone adds a new primitive type.

> Are we intending to leave this, and then do another annotations pass?

Yeah, that's what part 4 is about.  I could move this one bit to part 4 too, but it was nice to have something to test in part 3.... ;)

> This wants some static asserts about the state of Any with respect to the other values.

Hmm.  Any idea where is a good place to put those?

Comment 10

[Boris Zbarsky [:bzbarsky]]

> Boy, this makes me want to split the DOM stuff out of MCall

Yeah, I wasn't quite willing to tackle that.  But it would be nice.  ;)

> nit: multi-line for has brace on next line.
> nit: multi-line if conditions have brace on next line.

Both done.

Comment 11

[Eric Faust [:efaust]]

(In reply to Boris Zbarsky [:bz] from comment #9)
> Hmm.  Any idea where is a good place to put those?

How about JSJitInfo::staticAsserts()?

It looks like there is some precedent for doing this scattered around the engine already.

If we don't like that, we can always define a constructor that does nothing but the static assert if we want.

Comment 12

[Boris Zbarsky [:bzbarsky]]

static void staticAsserts() it is.

Comment 13

[Peter Van der Beken [:peterv]]

Comment on attachment 831727 [details] [diff] [review]
part 2.  Store information about argument types in jitinfo.

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

::: dom/bindings/Codegen.py
@@ +6286,5 @@
> +        if t.isUnion():
> +            u = t.unroll();
> +            type = "JSJitInfo::Null" if u.hasNullableType else ""
> +            return reduce(CGMemberJITInfo.getSingleArgType,
> +                          u.flatMemberTypes, type)

Could we do |return "JSJitInfo::ArgType(" + reduce(...) + ")"| instead of adding that inside getSingleArgType?

::: js/src/jsfriendapi.h
@@ +1442,5 @@
> +        // And derived types
> +        Numeric = Integer | Double,
> +        // Should "Primitive" use the WebIDL definition, which
> +        // excludes string and null, or the typical JS one that includes them?
> +        Primitive = Integer | Double | Boolean | Null | String,

Maybe use Numeric instead here?

Comment 14

[Boris Zbarsky [:bzbarsky]]

> Could we do |return "JSJitInfo::ArgType(" + reduce(...) + ")"|

Yes.   I guess ArgType(foo) with no '|' inside will never happen since we're a union after all.

> Maybe use Numeric instead here?

Done.

Thank you for the reviews!

Comment 15

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c4c1d2623227
https://hg.mozilla.org/integration/mozilla-inbound/rev/8446270d5511
https://hg.mozilla.org/integration/mozilla-inbound/rev/c643b87f2d62
https://hg.mozilla.org/integration/mozilla-inbound/rev/1df327c93b8d

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c4c1d2623227
https://hg.mozilla.org/mozilla-central/rev/8446270d5511
https://hg.mozilla.org/mozilla-central/rev/c643b87f2d62
https://hg.mozilla.org/mozilla-central/rev/1df327c93b8d

Comment 17

[Boris Zbarsky [:bzbarsky]]

Documented at https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Pure