Closed Bug 1022356 Opened 5 years ago Closed 5 years ago

Typed objects: complete transition to structural array types

Categories

(Core :: JavaScript Engine, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED WONTFIX
mozilla33

People

(Reporter: nmatsakis, Assigned: nmatsakis)

References

Details

Attachments

(3 files)

Due to upcoming uplift, I am opening a new bug for the remaining work on bug 973238. The goals remain the same as that bug.
Blocks: 973238
Assignee: nobody → nmatsakis
Move from TypeDescrSet, which simply tracked a set of descriptors, to TypedObjectPrediction, which tracks the intersection of all observed types. There are several reasons to make this change:

1. At the end of this patch series, we won't have access to the type descriptors anymore (though at this point in the series, they are still accessible since typed object prototypes (TypedProto) and descriptors still have a 1:1 relationship). So something has to change.

2. The TypeDescrSet system is simple but its size grows as O(n) with the number of observed descriptors. The size of a TypedObjectPrediction is O(1) yet it is basically as descriptive.

Note: To some extent, the design of the TypedObjectPrediction is perhaps a bit speculative -- specifically the detection of prefixes. This is partially a proof-of-concept. Since there is no typed object code In The Wild, it's hard to predict what patterns we will want to optimize, but I expect it will be common to have functions that operate over many kinds of structs which all share a common prefix of fields. In any case, it's easy enough to change the specifics of this definition as we go.
Attachment #8436610 - Flags: review?(jdemooij)
Comment on attachment 8436610 [details] [diff] [review]
Bug1022356-Part01.diff

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

Shu, can you review this patch? I've many reviews and these big patches can take a lot of time, so I'm trying to spread it out a bit.

After talking to Niko we decided you or Eric are probably good candidates for the TypedObject changes.
Attachment #8436610 - Flags: review?(jdemooij) → review?(shu)
Comment on attachment 8436610 [details] [diff] [review]
Bug1022356-Part01.diff

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

Overall this was surprisingly straightforward. The translation from using TypedProtoSet to TypedObjectPrediction was pretty much 1-1 so I didn't read the existing logic there too closely. I have one point of confusion for the prefix case in TypedObjectPrediction, but otherwise it seemed clear.

r=me with the Prefix case fixed.

::: js/src/builtin/TypedObject.h
@@ +1059,5 @@
>  
> +inline js::type::Kind
> +js::TypedProto::kind() const {
> +    return typeDescr().kind();
> +}

Can this be defined inline of the class decl above?

::: js/src/jit/IonBuilder.cpp
@@ +10208,1 @@
>      }

Style nit: remove { } for single-line body of if.

::: js/src/jit/TypedObjectPrediction.cpp
@@ +43,5 @@
> +      case Inconsistent:
> +        return; // keep same state
> +
> +      case Proto:
> +      {

Style nit: I *think* we write Proto: {

@@ +55,5 @@
> +            return inconsistent();
> +
> +        const StructTypeDescr &structDescr = proto.typeDescr().as<StructTypeDescr>();
> +        const StructTypeDescr &currentDescr = data_.proto->typeDescr().as<StructTypeDescr>();
> +        findPrefix(structDescr, currentDescr, SIZE_MAX);

I'm not understanding the logic for the Prefix case.

- findPrefix asserts that predictionKind() == Prefix, but we'll be calling it here while it's == Proto.
- Is it useful to set a 0-length prefix and should that be allowed?

@@ +269,5 @@
> +    MOZ_ASSUME_UNREACHABLE("Bad prediction kind");
> +}
> +
> +TypeDescr &
> +getArrayElementType(const TypeDescr &descr) {

static TypeDescr &
GetArrayElemenType(...)
{

@@ +339,5 @@
> +        break;
> +
> +      case TypedObjectPrediction::Proto:
> +        return hasFieldNamedPrefix(
> +            proto().typeDescr().as<StructTypeDescr>(), SIZE_MAX,

Why is SIZE_MAX passed her for fieldCount?

::: js/src/jit/TypedObjectPrediction.h
@@ +68,5 @@
> +        // Multiple different struct types flow into the same location,
> +        // but they share fields in common. Prefix indicates that the first
> +        // N fields of some struct type are known to be valid. This occurs
> +        // in a subtyping scenario.
> +        Prefix

Nit: perhaps order variants according to their precision?

@@ +83,5 @@
> +        PrefixData prefix;
> +    };
> +
> +  private:
> +    friend class NonemptyTypedObjectPrediction;

Nit: NonEmpty

@@ +92,5 @@
> +    PredictionKind predictionKind() const {
> +        return kind_;
> +    }
> +
> +    void inconsistent() {

I'd prefer this be named setInconsistent or markInconsistent or something with a verb.

@@ +134,5 @@
> +
> +    template<typename T>
> +    typename T::Type extractType() const;
> +
> +    bool hasFieldNamedPrefix(const StructTypeDescr &descr,

Maybe hasFieldNamedInPrefix?

@@ +177,5 @@
> +    // object instance. Returns null if we cannot or if the typed
> +    // object is of scalar/reference kind, in which case instances are
> +    // not objects and hence do not have a (publicly available)
> +    // prototype.
> +    const TypedProto *knownPrototype() const;

In general nullable getters are prefixed with get.

@@ +182,5 @@
> +
> +    ///////////////////////////////////////////////////////////////////////////
> +    // Queries that are valid if not useless.
> +
> +    type::Kind kind() const;

I find the naming of kind() vs predictionKind() confusing when reading code. Maybe rename predictionKind() or add more qualifiers to kind()? I have no good suggestions though.

@@ +210,5 @@
> +    // Returns true if the length of the array is statically known,
> +    // and sets |*length| appropriately. Otherwise returns false.
> +    bool hasKnownArrayLength(int32_t *length) const;
> +
> +    // Returns a prediction for the array element temp, if any.

s/temp/type
Attachment #8436610 - Flags: review?(shu) → review+
(In reply to Shu-yu Guo [:shu] from comment #3)
> I'm not understanding the logic for the Prefix case.
> 
> - findPrefix asserts that predictionKind() == Prefix, but we'll be calling
> it here while it's == Proto.

Good point. The assertion is flawed. Clearly, though, more tests are needed. I'm debating about just pulling the logic around Prefix out, since it's kind of premature optimization.

The logic of findPrefix() was supposed to be: set the prediction to be the common set of fields from two struct type descriptors, considering at most the first `max` fields.

> - Is it useful to set a 0-length prefix and should that be allowed?

It's not particularly useful, no. It should probably just change to inconsistent in that case. But it also doesn't really do any harm I don't think.
Try run for cleaned up version of Part 1: https://tbpl.mozilla.org/?tree=Try&rev=d114c0107574
At this stage in the patch series, there is still a 1-to-1 relationship between typed prototypes and type descriptors. The goal of this patch is to stop taking advantage of that link in various places in the self-hosted code.

One effect of this is that, in the case of arrays, the |objectType()| function must create a new fixed-length array type descriptor. This is per the (planned) spec.
Attachment #8443159 - Flags: review?(efaustbmo)
The current self-hosted code hardcodes a bunch of accessors that just (rather unsafely) pull data out of random slots in typed objects. This has led to some security flaws. The goal of this patch is to use a safer pattern that asserts the objects have the expected class before poking at their reserved slots -- but in such a way that TI ought to eliminate the cost of the checks entirely once jitted code kicks in (presuming that all the assertions will succeed, of course).
Attachment #8443160 - Flags: review?(efaustbmo)
https://hg.mozilla.org/mozilla-central/rev/352807a4b5bc
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Marking as reopened; I had intended to designate this bug as [leave-open].
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [leave-open]
("leave-open" has been promoted to a keyword; no need to use the whiteboard for it anymore.)
Keywords: leave-open
Whiteboard: [leave-open]
Depends on: 1029702
Depends on: 1029126
No longer depends on: 1029702
Comment on attachment 8443160 [details] [diff] [review]
Bug1022356-Part03.diff

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

This does indeed look much safer. It also doesn't look that hard for TI to tell us everything we need to know about these.
Attachment #8443160 - Flags: review?(efaustbmo) → review+
Comment on attachment 8443159 [details] [diff] [review]
Bug1022356-Part02.diff

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

I see what you mean about the sized and unsized arrays. It would be nice to see them go.
Attachment #8443159 - Flags: review?(efaustbmo) → review+
After some discussion with bhackett, we've decided to close this bug and suspend this line of work. I don't have the time (clearly) to rebase and land these patches right now, and he's going to work on optimizing what exists, and then adapting as needed.
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → WONTFIX
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.