Closed Bug 880208 Opened 7 years ago Closed 5 years ago

[PJS] Add UnsafeGet instrinsics


(Core :: JavaScript Engine, defect)

Not set





(Reporter: nmatsakis, Assigned: nmatsakis)



(Whiteboard: [leave open])


(2 files, 1 obsolete file)

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.
Attachment #762075 - Flags: review?(kvijayan)
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.
Attachment #762075 - Flags: review?(kvijayan) → review+
Assignee: general → nmatsakis
Address djvj's comments.
Attachment #762075 - Attachment is obsolete: true
Attachment #762449 - Flags: review+
Depends on: 883395
Unfortunately, I had to back this out in:

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).
Attached file 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)
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
This was backed out, see above.
Resolution: FIXED → ---
Whiteboard: [leave open]
Target Milestone: mozilla24 → ---
Depends on: 883524
No longer depends on: 883524
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").
(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #9)
> This was backed out, see above.

Backout made mozilla-central:
Blocks: PJS
Closed: 7 years ago5 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.