Investigate having a Baseline IC for adding a property even if that involves slot allocation/reallocation

RESOLVED FIXED in Firefox 54

Status

()

P3
normal
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: bzbarsky, Assigned: jandem)

Tracking

(Blocks: 3 bugs, {perf})

unspecified
mozilla54
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(1 attachment)

See bug 1091795 comment 6: a bunch of unoptimizable accesses in octane-box2d are due to us going from 0 to 8 dynamic slots on a property add.
Blocks: 1218972
Blocks: 1307062
Priority: -- → P5
(Assignee)

Comment 1

2 years ago
I should be able to fix this pretty soon, either as part of bug 1326067 or as a followup.
Depends on: 1326067
Priority: P5 → P3
Keywords: perf
(Assignee)

Comment 2

2 years ago
Posted patch PatchSplinter Review
This adds a new CacheIR op, AllocateAndStoreDynamicSlot. It (re)allocates dynamic slots and then does the store like AddAndStoreDynamicSlot. Like the current Ion IC code for this, we call NativeObject::growSlotsDontReportOOM.

For the micro-benchmark below, I get:

--no-ion before: 490 ms
--no-ion after:   63 ms
<no flags>:       40 ms

It's pretty silly Ion needs ICs for AddSlot btw. This is one of the things where compiling CacheIR to MIR could be a big win.

function f() {
    for (var i=0; i<1000000; i++) {
        var o = {x: 1};
        o.x2 = i;
        o.x3 = i;
        o.x4 = i;
        o.x5 = i;
        o.x6 = i;
        o.x7 = i;
    }
}
var t = new Date;
f();
print(new Date - t);
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Attachment #8831979 - Flags: review?(evilpies)
Comment on attachment 8831979 [details] [diff] [review]
Patch

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

Great improvements! We really should do something about Ion though. 
With code like your example, do we end up calling NativeObject::growSlotsDontReportOOM for every property that we are adding?

::: js/src/jit/BaselineCacheIRCompiler.cpp
@@ +802,5 @@
> +        LiveRegisterSet save(regs.asLiveSet());
> +        masm.PushRegsInMask(save);
> +
> +        // Use ICStubReg as second scratch.
> +        if (!save.has(ICStubReg))

Can we add ICStubReg to |save| before PushRegsInMask?
Attachment #8831979 - Flags: review?(evilpies) → review+
Blocks: 1259927
(Assignee)

Comment 4

2 years ago
(In reply to Tom Schuster [:evilpie] from comment #3)
> With code like your example, do we end up calling
> NativeObject::growSlotsDontReportOOM for every property that we are adding?

No only when we actually have to allocate/reallocate the slots (I think we allocate 8 dynamic slots when we need one, etc).

Comment 5

2 years ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a2caf5127bf4
Make CacheIR AddProp stub support dynamic slot (re)allocation. r=evilpie

Comment 6

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a2caf5127bf4
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Depends on: 1344334
You need to log in before you can comment on or make changes to this bug.