Open Bug 1155340 Opened 6 years ago Updated 9 months ago

Generate better jitinfo for autogenerated maplike/setlike methods

Categories

(Core :: DOM: Bindings (WebIDL), defect, P3)

x86
macOS
defect

Tracking

()

Tracking Status
firefox40 --- affected

People

(Reporter: bzbarsky, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Bug 1123516 adds maplike/setlike by desugaring them into IDL operations.  But the value-returning operations use "any" as the type, since that makes it easy to have them just call through to the JS engine Map/Set stuff.

Unfortunately, this means our jitinfo just claims we return Value.  In practice, we may know more about what can end up getting stored in the Map/Set.  So it might be nice to hang the actual return type off the IDLOperation somewhere and use it in jitinfo generation.

One option would be to use the specific type for the IDLOperation and to fix codegen for these special methods to ignore the return type and just treat it as "any".

Another option would be to change this bit in jitinfo codegen:

            sigs = self.member.signatures()

and then the use of [s[0] for s in sigs] as the returnTypes argument to defineJitInfo to use a new method on IDLOperation (getJitInfoReturnTypes()?) that normally returns exactly (overload.returnType for overload in self._overloads) but for the methods generated by maplike/setlike returns a single-element list containing the type stored in the Map/Set.

The former approach encapsulates the weirdness more to the maplike/setlike stuff without producing extra coupling between codegen and the parser.  The latter approach may be easier to implement in practice.
Mentor: bzbarsky
For now, this is only the |get| method.

Unfortunately this doesn't change the generated JitInfo, since the return type
needs to be nullable to take into account the missing key case.

Still worth it, I guess, since it improves the generated code a bit.

Review commit: https://reviewboard.mozilla.org/r/64776/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64776/
Attachment #8771734 - Flags: review?(bzbarsky)
Assignee: nobody → ealvarez
Hmm.  I had completely forgotten about the "missing key" case.  :(

Given that, "any" might be the only thing we have right now that can represent this.  Using a nullable version of the return type is _not_ the same thing; if we ever fixed our jitinfo stuff it would give the wrong type info to the jit (e.g. would claim the return value is integer-or-null whereas it's actually integer-or-undefined).

To really get mileage out of this we'd need to change jitinfo to store a set of JSValueTypes, not just one.  And then store { JSVAL_TYPE_UNDEFINED, whatever-the-actual-type-is }.  And then see whether the jits can make use of this information.  I'm not sure whether it's worth it.  :(

What was the observed improvement to the generated code, if I might ask?
Flags: needinfo?(ealvarez)
Attached file generated.diff
Sure, basically it avoids generating some noop CallerSubsumes checks, though I agree since they are effectively dead code, and it's not code intended to be read, might be sensible leaving things as-is...

Sorry for the Nullable thing.
Flags: needinfo?(ealvarez)
Ah, I see.  So for the specific case of JS-implemented maplikes we get better codegen... but also incorrect behavior, because the undefined gets turned into null coming back out through the bindings.

I don't think this bug is possible to fix until we have "void" represented as an actual IDL type, so we can have things returning "(Node or void)" or some such.  Sorry I didn't catch that when filing it.  :(
Attachment #8771734 - Flags: review?(bzbarsky) → review-
Comment on attachment 8771734 [details]
Bug 1155340: bindings: Provide better info for value-returning maplike methods.

https://reviewboard.mozilla.org/r/64776/#review62004

This actually gives incorrect behavior; see discussion on the bug.
I don't think I'll look into this near-term.
Assignee: emilio → nobody
Priority: -- → P3
Component: DOM → DOM: Bindings (WebIDL)
Mentor: bzbarsky
You need to log in before you can comment on or make changes to this bug.