1996839 - Resolve hooks can prevent transpiling property lookups

Status: NEW

(Core :: JavaScript Engine: JIT, task, P3)

Description

[Jan de Mooij [:jandem]]

When we don't attach an IC stub in Baseline because we still need to invoke a resolve hook, we treat this as an IC 'failure' and this then blocks CacheIR transpilation in Warp. On Speedometer 3 and JetStream 3 I see this show up a number of times, especially for GetGName.

The micro-benchmark below takes ~250 ms on my machine, but with the i > 20 changed to i > 5 (or with --blinterp-warmup-threshold=25) it takes 93 ms.

It'd be good to fix this, also because this bug means that changes to warmup thresholds (or more eager Baseline compilation with JIT hints) can have a much larger performance impact than you'd expect.

function f() {
  var t = Date.now();
  var res;
  for (var i = 0; i < 100_000_000; i++) {
    if (i > 20) {
      res = Math.abs(1);
    }
  }
  print(Date.now() - t);
  return res;
}
f();

Comment 1

[Jan de Mooij [:jandem]]

One option here is to use AttachDecision::TemporarilyUnoptimizable when we have a resolve hook for the property. Two downsides I see with that approach:

1. Plumbing this through from LookupPropertyPure is doable but a bit of a pain.
2. ClassMayResolveId is not a precise check because it depends on the mayResolve implementation of the JSClass. Some classes might not even have a mayResolve hook. This could result in repeatedly returning TemporarilyUnoptimizable without attaching anything, instead of maybe attaching a more generic stub.

Comment 2

[Iain Ireland [:iain]]

Would it work to reorder DoGetNameFallback to get the result before attaching the stub? That way we should always trigger resolve hooks first. It speeds the testcase up locally; I'm running a try build to see if it breaks anything.

Comment 3

[Jan de Mooij [:jandem]]

(In reply to Iain Ireland [:iain] from comment #2)

Would it work to reorder DoGetNameFallback to get the result before attaching the stub? That way we should always trigger resolve hooks first. It speeds the testcase up locally; I'm running a try build to see if it breaks anything.

It's complicated when we have getters, especially for ops such as GetProp. Imagine you have a getter function that adds a data property or changes the object's shape in some other way. The shape we see when we attach the call-getter stub then no longer matches the original before-getter-call stub so the stub will always fail.

Comment 4

[Iain Ireland [:iain]]

Hmm, interesting. We already have AttachDecision::Deferred to handle the AddSlot case for SetProp (where we need the shape before and after). We could maybe reuse that here. If there's a resolve hook and no failures, return Deferred and try attaching again afterwards.

It's maybe also worth noting that the example testcase uses the GetName IRGenerator, not the GetProp IRGenerator, which might be a little simpler. Is the same issue also appearing with resolve hooks in GetProp?

Comment 5

[Jan de Mooij [:jandem]]

(In reply to Iain Ireland [:iain] from comment #4)

Hmm, interesting. We already have AttachDecision::Deferred to handle the AddSlot case for SetProp (where we need the shape before and after). We could maybe reuse that here. If there's a resolve hook and no failures, return Deferred and try attaching again afterwards.

I don't want to add more uses of that mechanism, but there aren't any great options here. I'm now leaning towards going back to TemporarilyUnoptimizable but maybe have a different kind that means: TemporarilyUnoptimizable the first time this happens, else give up (NoAction) or maybe count as a failure.

In CanAttachGlobalName we don't check for resolve hooks at all currently and that should probably be fixed too.

It's maybe also worth noting that the example testcase uses the GetName IRGenerator, not the GetProp IRGenerator, which might be a little simpler. Is the same issue also appearing with resolve hooks in GetProp?

Not as frequently but it can show up there, for example like this:

function Foo() {}
function f() {
  var t = Date.now();
  var res;
  for (var i = 0; i < 100_000_000; i++) {
    if (i > 20) {
      res = Foo.prototype;
    }
  }
  print(Date.now() - t);
  return res;
}
f();

And ideally window.Math and globalThis.Math would be handled the same way as Math.