Closed Bug 1383777 Opened 2 years ago Closed 2 years ago

Support additional idempotent ICs

Categories

(Core :: JavaScript Engine: JIT, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: bhackett, Assigned: bhackett)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

GetPropIRGenerator::tryAttachIdempotentStub() has this comment in it:

    // For idempotent ICs, only attach stubs for plain data properties.
    // This ensures (1) the lookup has no side-effects and (2) Ion has complete
    // static type information and we don't have to monitor the result. Because
    // of (2), we don't support for instance missing properties or array
    // lengths, as TI does not account for these cases.

On Speedometer v2, accessing missing properties and array lengths on idempotent ICs account for 1444 invalidations, or 69% of the total number of idempotent IC invalidations.  These property accesses do not have side effects and can be performed in an idempotent fashion.  (2) above is not a fundamental restriction, static TI information isn't necessary as long as the ICs produce values which the MIR node's consumers are designed to handle.
Attached patch patchSplinter Review
This patch keeps track of which idempotent caches may produce undefined or int32 values, allowing missing property and object length ICs to be attached.  This produces the expected reduction in invalidations on speedometer.
Assignee: nobody → bhackett1024
Attachment #8889554 - Flags: review?(jdemooij)
Comment on attachment 8889554 [details] [diff] [review]
patch

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

Sweet.

::: js/src/jit/CacheIR.h
@@ +1141,5 @@
>  
>  enum class CanAttachGetter { Yes, No };
>  
> +// Flags used to describe what values a GetProperty cache may produce.
> +enum GetPropertyResultFlags {

enum class

The patch already uses the GetPropertyResultFlags:: prefix so it should just compile hopefully.

::: js/src/jit/IonIC.cpp
@@ +140,5 @@
>          jsbytecode* pc = ic->idempotent() ? nullptr : ic->pc();
>          bool isTemporarilyUnoptimizable = false;
>          GetPropIRGenerator gen(cx, outerScript, pc, ic->kind(), ic->state().mode(),
> +                               &isTemporarilyUnoptimizable, val, idVal, val, canAttachGetter,
> +                               ic->resultFlags());

canAttachGetter is defined like this:

    CanAttachGetter canAttachGetter =
        ic->monitoredResult() ? CanAttachGetter::Yes : CanAttachGetter::No;

So we could remove CanAttachGetter and replace its uses in CacheIR.cpp with the resultFlags & ...Monitored check, to simplify the GetProp code.
Attachment #8889554 - Flags: review?(jdemooij) → review+
Pushed by bhackett@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/08bef58cdb9d
Support idempotent ICs that access missing properties and object lengths, r=jandem.
https://hg.mozilla.org/mozilla-central/rev/08bef58cdb9d
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Depends on: 1384737
Depends on: 1388587
You need to log in before you can comment on or make changes to this bug.