880208 - [PJS] Add UnsafeGet instrinsics

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[Niko Matsakis [:nmatsakis]]

When implementing array-like types in self-hosted code, and in particular multi-dimensional arrays, it is helpful to be able to disable redundant bounds checking. It is also helpful to be able to give hints as to what data is fully encapsulated and will not change. To this end I have added two intrinsics:

    UnsafeGetElement(arr, idx)
    UnsafeGetImmutableElement(arr, idx)

Both are equivalent to arr[idx], but (in ion-compiled code) the former skips bounds checks, and the latter skips bounds checks and also assumes that the data cannot be mutated by stores.

Comment 1

[Niko Matsakis [:nmatsakis]]

Attachment: Add UnsafeGet and UnsafeGetImmutable.

Comment 2

[Kannan Vijayan [:djvj]]

Comment on attachment 762075 [details] [diff] [review]
Add UnsafeGet and UnsafeGetImmutable.

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

::: js/src/ion/MCallOptimize.cpp
@@ +1060,5 @@
>  
>  IonBuilder::InliningStatus
> +IonBuilder::inlineUnsafeGetElement(CallInfo &callInfo,
> +                                   GetElemSafety safety)
> +{

Seems like |safety| can only be one of |GetElem_Unsafe| or |GetElem_UnsafeImmutable|, and never |GetElem_Normal|.

Would be good to assert that here.

Comment 3

[Niko Matsakis [:nmatsakis]]

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

Comment 4

[Niko Matsakis [:nmatsakis]]

Attachment: Add UnsafeGet and UnsafeGetImmutable.

Address djvj's comments.

Comment 5

[Niko Matsakis [:nmatsakis]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/18c1fd169792

Comment 6

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

Unfortunately, I had to back this out in:

https://hg.mozilla.org/integration/mozilla-inbound/rev/a8e3d80187d1

This caused breakage in jsfunfuzz itself (see bug 883395), another sample testcase is also coming up. There was no response on IRC when I pinged for some time, and I also spoke to djvj and Luke (in-person).

The testcase in bug 883395 also looks like it could cause real-world breakage, so backing out to avoid having to deal with nightly crashes. Please feel free to request for fuzzing of patches from :decoder or me (:gkw).

Comment 7

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

Attachment: Debug stack

function exploreDeeper(a, n) {
    var hns = Object.getOwnPropertyNames(a);
    for (var j = 0; j < hns.length; ++j) {
        var hn = hns[j];
        try {
            va = a[hn];
        } catch (e) {}
    }
}
gns = Object.getOwnPropertyNames(this);
for (var i = 0; gns.length; ++i) {
    var gn = gns[i];
    if (gn.charCodeAt(0) < 0x60) {
        g = this[gn];
        if (g.toString().indexOf("[native code]") != -1) {
            exploreDeeper(g.prototype, +".e");
        }
    }
}

asserts rev 7fc2d9ea82b6 at Assertion failure: args[arri].isObject(), at vm/SelfHosting.cpp - tested on a threadsafe 64-bit debug shell on Mac 10.8 (--enable-more-deterministic is not required)

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/18c1fd169792

Comment 9

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

This was backed out, see above.

Comment 10

[Niko Matsakis [:nmatsakis]]

The problem is that the ParallelArray code doesn't include guards to check that `this` is always a parallel array object (and, presumably, to take a slow path in the event that it's not, so as to be "proxy-proof").

Comment 11

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

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #9)
> This was backed out, see above.

Backout made mozilla-central:

http://hg.mozilla.org/mozilla-central/rev/a8e3d80187d1