898349 - [binary data] Integrate binary data objects into the JIT

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Niko Matsakis [:nmatsakis]]

In order to efficiently handle binary data objects, the JIT will need access to the type information for binary data objects. The plan is to put this information into the type objects, as described here:

http://smallcultfollowing.com/babysteps/blog/2013/07/19/integrating-binary-data-and-type-inference-in-spidermonkey/

This bug pertains to accessing this information and compiling accesses to properties etc to be simple loads where possible, as well as integrating into ICs.

Comment 1

[Niko Matsakis [:nmatsakis]]

Attachment: Part 1: Add TypeRepresentationSets and supports loads/stores from scalar properties

jandem -- This is an early version of the patch for integrating typed objects into ion. It only supports loads and stores from scalar properties right now. I just wanted to get any early feedback on the approach. More details on my plan are available at http://smallcultfollowing.com/babysteps/blog/2013/07/19/integrating-binary-data-and-type-inference-in-spidermonkey/

Comment 2

[Niko Matsakis [:nmatsakis]]

Attachment: Bug898349-Part1-Scalar.diff

This is a more complete patch than the previous one. We now support loads and stores to scalar fields, and also optimizing away intermediate typed objects (i.e., the intermediate object created for `a.b` in an expression like `a.b.c`). More work is needed to improve and optimize other usage patterns -- particularly typed object creation but also assignment of non-scalar properties -- but those can wait for followup bugs. Before landing I plan to cook up some more tests, and probably a benchmark for AWFY. Still I thought I'd put it up for review now and get it in the queue.

Comment 3

[Jan de Mooij [:jandem]]

Comment on attachment 799680 [details] [diff] [review]
Bug898349-Part1-Scalar.diff

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

Looks great, sorry it took me so long I expected this to be more complicated.

Comments below are mostly nits. If you post a new patch addressing these I promise faster review times :)

::: js/src/jit/IonBuilder.cpp
@@ +8176,5 @@
> +}
> +
> +bool
> +IonBuilder::getPropTryScalarPropOfTypedObject(
> +    bool *emitted,

Nit: can  you indent these arguments like you did with getPropTryTypedObject?

@@ +8185,5 @@
> +    // Must always be loading the same scalar type
> +    if (fieldTypeReprs.length() != 1)
> +        return true;
> +    ScalarTypeRepresentation *fieldTypeRepr =
> +        fieldTypeReprs.get(0)->asScalar();

Nit: fits on one line.

@@ +8197,5 @@
> +    MDefinition *owner, *ownerOffset;
> +    loadTypedObjectData(typedObj, fieldOffset, &owner, &ownerOffset);
> +
> +    // Load the element data.
> +    MTypedObjectElements *elements = new MTypedObjectElements(owner);

Nit: MTypedObjectElements::New(owner) for consistency.

@@ +8206,5 @@
> +    // and the instruction is not known to return a double.
> +    bool allowDouble =
> +        resultTypes->hasType(types::Type::DoubleType());
> +    MIRType knownType =
> +        MIRTypeForTypedArrayRead(fieldTypeRepr->type(), allowDouble);

Nit: these also fit on one line I think (limit is 99 for code, 79 for comments).

@@ +8215,5 @@
> +    MConstant *alignment =
> +        MConstant::New(Int32Value(fieldTypeRepr->alignment()));
> +    current->add(alignment);
> +    MDiv *scaledOffset =
> +        MDiv::NewAsmJS(ownerOffset, alignment, MIRType_Int32);

Same here.

@@ +8230,5 @@
> +}
> +
> +bool
> +IonBuilder::getPropTryComplexPropOfTypedObject(
> +    bool *emitted,

Nit: see previous indentation comment.

@@ +8547,5 @@
>  
>      types::StackTypeSet *objTypes = obj->resultTypeSet();
>      bool barrier = PropertyWriteNeedsTypeBarrier(cx, current, &obj, name, &value);
>  
> +    // Try to emit loads from known binary data blocks

Nit: s/loads from/stores to

@@ +8715,5 @@
> +
> +    // OK!
> +    *emitted = true;
> +
> +    MTypedObjectElements *elements = new MTypedObjectElements(obj);

Nit: MTypedObjectElements::New(obj) for consistency.

@@ +9520,5 @@
> +    if (fieldTypeReprs->empty())
> +        return false;
> +
> +    // Field offset must be representable as signed integer.
> +    if (offset >= (size_t) INT_MAX) {

Nit: size_t(INT_MAX), int32_t(offset).

@@ +9538,5 @@
> +{
> +    // Load list of field type objects.
> +
> +    MInstruction *fieldTypes =
> +        MLoadFixedSlot::New(typeObj, SLOT_STRUCT_FIELD_TYPES);

Nit: fits on one line, also a few more times below.

::: js/src/jit/MIR.h
@@ +1485,5 @@
> +                           MDefinition *offset)
> +      : MTernaryInstruction(type, owner, offset),
> +        set_(set)
> +    {
> +        setResultType(MIRType_Object);

You probably want setMovable(); as well (see also the comment below).

@@ +1509,5 @@
> +        return this;
> +    }
> +
> +    virtual AliasSet getAliasSet() const {
> +        return AliasSet::None();

If we do a.b.c, we don't have to be worried about some other instruction doing a.b = foo, or whatever the syntax is?

Just checking it's fine for instance to hoist NewDerivedTypedObject out of loops no matter what the loop does. If that's not safe, this should probably use Load(TypedObjectField) or something.

Also, shouldn't this instruction override congruentTo? That way GVN can eliminate redundant instructions.

::: js/src/jit/TypeRepresentationSet.cpp
@@ +22,5 @@
> +TypeRepresentationSetHasher::hash(TypeRepresentationSet key)
> +{
> +    HashNumber hn = mozilla::HashGeneric(key.length());
> +    for (size_t i = 0; i < key.length(); i++) {
> +        hn = mozilla::AddToHash(hn, (uintptr_t) key.get(i));

Nit: single line body so no {}, also C++-style cast: uintpr_t(key.get(i)).

@@ +82,5 @@
> +        uintptr_t entryiaddr = (uintptr_t) entries_[i];
> +        if (entryiaddr == typeReprAddr) {
> +            // typeRepr already present in the set
> +            return true;
> +        } else if (entryiaddr < typeReprAddr) {

Nit: no else after return.

::: js/src/jit/TypeRepresentationSet.h
@@ +15,5 @@
> +namespace jit {
> +
> +class IonBuilder;
> +class TypeRepresentationSet;
> +

There should be a comment around here explaining what a TypeRepresentationSet is and why the JIT needs it. We also want to make it clear this is only used for binary data / typed objects and is not some generic TI mechanism or something. Readers unfamiliar with the code will appreciate that before they dive in too far :)

Comment 4

[Tom S [:evilpie]]

Comment on attachment 799680 [details] [diff] [review]
Bug898349-Part1-Scalar.diff

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

::: js/src/jit/VMFunctions.h
@@ +662,5 @@
>  
>  bool InitBaselineFrameForOsr(BaselineFrame *frame, StackFrame *interpFrame,
>                               uint32_t numStackValues);
>  
> +JSObject *CreateDerivedTypedObj(JSContext *cx, HandleObject type,

Object please.

Comment 5

[Niko Matsakis [:nmatsakis]]

Attachment: Bug898349-Part1-Scalar.diff

Addressed jandem's comments, submitting for re-review.

Comment 6

[Niko Matsakis [:nmatsakis]]

Attachment: Bug898349-Part1-Scalar.diff

Rebased, re-requesting review, requesting fuzzing. Base revision is now 42242953371b

Comment 7

[Niko Matsakis [:nmatsakis]]

Comment on attachment 806516 [details] [diff] [review]
Bug898349-Part1-Scalar.diff

Argh. Marking as obsolete since testing revealed some merge errors that had slipped through when I rebased. New patch coming soon.

Comment 8

[Niko Matsakis [:nmatsakis]]

Attachment: Bug898349-Part1-Scalar.diff

Rebased on to bb60799d72fe. Requesting fuzz, review.

Comment 9

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 806608 [details] [diff] [review]
Bug898349-Part1-Scalar.diff

Nothing bad found overnight.

Comment 10

[Jan de Mooij [:jandem]]

Comment on attachment 806608 [details] [diff] [review]
Bug898349-Part1-Scalar.diff

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

::: js/src/jit/IonBuilder.cpp
@@ +8302,5 @@
> +    // Reading from an Uint32Array will result in a double for values
> +    // that don't fit in an int32. We have to bailout if this happens
> +    // and the instruction is not known to return a double.
> +    bool allowDouble =
> +        resultTypes->hasType(types::Type::DoubleType());

Nit: fits on one line, here and a few times below.

::: js/src/jit/TypeRepresentationSet.h
@@ +12,5 @@
> +#include "js/HashTable.h"
> +
> +// TypeRepresentationSet stores a set of TypeRepresentation* objects,
> +// representing the possible types of the binary data associated with
> +// a typed object value.  Often TypeRepresentationSets will be

Nice comment, thanks!

::: js/src/jscompartment.h
@@ +203,5 @@
>  
>      js::RegExpCompartment        regExps;
>  
>      /* Set of all currently living type representations. */
> +    js::TypeRepresentationHash   typeReprs;

Pre-existing, but we can have many compartments and typed objects are very rare, so especially on mobile using a pointer here may be worth it. I've no idea though how big a HashSet is so maybe it doesn't matter.

Comment 11

[Niko Matsakis [:nmatsakis]]

Try run: https://tbpl.mozilla.org/?tree=Try&rev=d32ae9a2f50c

Comment 12

[Niko Matsakis [:nmatsakis]]

Attachment: Bug898349-Part1-Scalar.diff

Carrying over r+ from jandem

Comment 13

[Niko Matsakis [:nmatsakis]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4c2b9302fae8

Comment 14

[Niko Matsakis [:nmatsakis]]

Comment on attachment 807946 [details] [diff] [review]
Bug898349-Part1-Scalar.diff

Clearing superreview flag that I accidentally set.

Comment 15

[Niko Matsakis [:nmatsakis]]

Attachment: Bug898349-Part2-Nits.diff

Forgot to address jandem's line-break nits.

Comment 16

[Niko Matsakis [:nmatsakis]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2f6ccca77bbd (nits)

Comment 17

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/4c2b9302fae8
https://hg.mozilla.org/mozilla-central/rev/2f6ccca77bbd