Closed Bug 938294 Opened 6 years ago Closed 6 years ago

Add a meaningful way to mark DOM methods [Pure]

Categories

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

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

(Keywords: dev-doc-complete)

Attachments

(4 files)

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.
Please note the comment about whether we're using the right definition of Primitive
Attachment #831727 - Flags: review?(peterv)
Attachment #831727 - Flags: review?(efaustbmo)
Part 4 still coming up, adding a bunch of [Pure] annotations.
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...
Blocks: 938355
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.
Attachment #831727 - Flags: review?(efaustbmo) → review+
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.
Attachment #831730 - Flags: review?(efaustbmo) → review+
> 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?
Flags: needinfo?(efaustbmo)
> 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.
(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.
Flags: needinfo?(efaustbmo)
static void staticAsserts() it is.
Blocks: 939581
Attachment #831726 - Flags: review?(peterv) → review+
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?
Attachment #831727 - Flags: review?(peterv) → review+
Attachment #831771 - Flags: review?(peterv) → review+
> 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!
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.