1169611 - IonBuilder::getPropTryConstant should not require a singleton result

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Jan de Mooij [:jandem]]

Attachment: Patch

IonBuilder::getPropTryConstant currently gives up if the observed result type is not a singleton object. It needs a singleton object to pass to testSingletonPropertyTypes.

Requiring a singleton there is not optimal when inlining a very polymorphic function, see bug 1169501.

The attached patch refactors testSingletonPropertyTypes to return the singleton object it finds and then we can use that without consulting the observed types. This fixes the problem in 1169501 and makes getPropTryConstant handle more cases on various Octane benchmarks like deltablue.

Comment 1

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0aac08013a2e

Comment 2

[Jan de Mooij [:jandem]]

This regressed Octane-pdfjs, because it exposed a pre-existing bug. In testSingletonPropertyTypes there's this hack:

    if (types->hasType(TypeSet::StringType())) {
        key = JSProto_String;
        *testString = true;
        break;
    }

Then, if testString == true, we insert a fallible unbox. But this, of course, is a recipe for uncontrolled bailouts.

Removing this code regresses Octane-crypto a lot though. I'll try to come up with a fix.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Follow-up patch

This patch fixes the Octane-pdfjs problem mentioned in comment 2. It removes the testObject and testString arguments from testSingletonPropertyTypes.

Instead of this, IonBuilder now tries to unbox the object input of a JSOP_GETPROP or JSOP_GETELEM based on Baseline feedback.

This fixes the Octane-pdfjs regression, does not regress Octane-crypto and should also fix bug 1131523 (older version of Lodash hit a somewhat-similar performance issue).

The bad news: the first patch is no longer as effective on the Flux benchmarks (bug 1169501), because the GETPROPs we care about there sometimes have {int32,string} inputs, and we'll no longer optimize that in testSingletonPropertyTypes. We'll have to fix that differently; will look into this next week.

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 8613226 [details] [diff] [review]
Follow-up patch

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

::: js/src/jit/BaselineInspector.cpp
@@ +761,5 @@
> +            stubType = MIRType_String;
> +            break;
> +
> +          default:
> +              printf("%s\n", ICStub::KindString(stub->kind()));

Oops, will remove this.

Comment 5

[Brian Hackett [Laid off!]]

Comment on attachment 8613226 [details] [diff] [review]
Follow-up patch

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

::: js/src/jit/IonBuilder.cpp
@@ +10015,5 @@
> +    MIRType type = inspector->expectedPropertyAccessInputType(pc);
> +    if (type == MIRType_Value || !def->mightBeType(type))
> +        return def;
> +
> +    MUnbox *unbox = MUnbox::New(alloc(), def, type, MUnbox::Fallible);

MUnbox* unbox

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2a1292a1d5c1

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0aac08013a2e
https://hg.mozilla.org/mozilla-central/rev/2a1292a1d5c1

Comment 8

[Hannes Verschore [:h4writer]]

Regression: http://arewefastyet.com/regressions/#/regression/1707288
Confirmed fixed: http://arewefastyet.com/regressions/#/regression/1716350

Similar results on other slaves.