Closed Bug 1332946 Opened 3 years ago Closed 3 years ago

CacheIR: IC for function.length

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: evilpie, Assigned: evilpie)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

It seems like we don't do a great job with GetProp on Function properties. On Facebook I see a lot of misses for Function#length and some for Function#prototype and Function#name. I have seen Function#prototype on a bunch of google sites as well and this is definitely seems like the most common GetProp IC that we are still missing.

On Facebook we attach ~23k GetProp ICs and all! of the missing ~1k ICs are those Function properties. We have really made great progress here.
Attached patch CacheIR: IC for function length. (obsolete) — Splinter Review
function.prototype seems difficult to implement after some consideration, we would have to allocate a new object and assign it to .prototype, probably not worth it after we stop trying to attach ICs after a certain limit.

This just implements function.length. It's more complex than I thought, but this should be a win on facebook.
Attachment #8829290 - Flags: feedback?(jdemooij)
Comment on attachment 8829290 [details] [diff] [review]
CacheIR: IC for function length.

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

::: js/src/jit/CacheIRCompiler.cpp
@@ +1537,5 @@
> +                      scratch,
> +                      Imm32(JSFunction::INTERPRETED_LAZY |
> +                            JSFunction::RESOLVED_LENGTH),
> +                      failure->label());
> +    // XXX could we get the length of lazy scripts?

I wonder if we're delazifying a lot of code on FB because of this. I do remember them using some tricks to trigger full parses, wasn't that done by looking up function.length?

@@ +1562,5 @@
> +    masm.loadPtr(Address(obj, JSFunction::offsetOfNativeOrScript()), scratch);
> +    masm.load16ZeroExtend(Address(scratch, JSScript::offsetOfFunLength()), scratch);
> +
> +    masm.bind(&done);
> +    EmitStoreResult(masm, scratch, JSVAL_TYPE_INT32, output);

Could you add a masm.printf(...) here, to see how often this stub succeeds? I think we can take the complexity if the stub succeeds most of the time.
Attachment #8829290 - Flags: feedback?(jdemooij) → feedback+
> Could you add a masm.printf(...) here, to see how often this stub succeeds? I think we can take the complexity if the
> stub succeeds most of the time.

Good idea! While looking at the my previous CacheIR Analyzer output, we failed to attach function.length around 1000 times, logging the number of successful times we executed the stub with masm.printf, gives me about 1700 now. We only fail to attach the stub about 50 times because of lazy functions.
Assignee: nobody → evilpies
Attachment #8829290 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8829405 - Flags: review?(jdemooij)
Keywords: leave-open
Comment on attachment 8829405 [details] [diff] [review]
CacheIR: IC for function.length

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

Good find.

::: js/src/jit/CacheIR.cpp
@@ +900,5 @@
> +GetPropIRGenerator::tryAttachFunction(HandleObject obj, ObjOperandId objId, HandleId id)
> +{
> +    // Function properties are lazily resolved so they might not be defined yet.
> +    // We however know that is a limited set of such properties and can attach
> +    // a generic stub for them.

Shouldn't we update this comment?

::: js/src/jit/CacheIRCompiler.cpp
@@ +1530,5 @@
> +
> +    // Get the JSFunction flags.
> +    masm.load16ZeroExtend(Address(obj, JSFunction::offsetOfFlags()), scratch);
> +
> +    // Function with lazy scripts don't store their length.

Nit: Functions

@@ +1552,5 @@
> +    masm.bind(&boundFunction);
> +    // Bound functions might have a non-int32 length.
> +    Address boundLength(obj, FunctionExtended::offsetOfExtendedSlot(BOUND_FUN_LENGTH_SLOT));
> +    masm.branchTestInt32(Assembler::NotEqual, boundLength, failure->label());
> +    masm.loadUnboxedValue(boundLength, MIRType::Int32, AnyRegister(scratch));

Maybe masm.unboxInt32(boundLength, scratch);
Attachment #8829405 - Flags: review?(jdemooij) → review+
(In reply to Jan de Mooij [:jandem] from comment #2)
> I do
> remember them using some tricks to trigger full parses, wasn't that done by
> looking up function.length?
> 

Yes. It's done for V8. From what I was told it wasn't really required for SpiderMonkey. But now it's used to get the argument counts to figure out which module are passed via arguments (vs. which ones are not) so it's no longer a V8 hack but FB is now using the value.

Once there's a try/inbound build I can run it through a static version of FB to see if the improvement is measurable. Just ni? me.
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6ed4bb887b1
CacheIR: IC for function.length. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6ed4bb887b120a2987c5484ef9e48a32840748b

As I said before function.prototype is a lot harder, while function.name is probably simple it's also less useful. From my point of view we can close this.
Flags: needinfo?(b56girard)
Keywords: leave-open
Summary: CacheIR: GetProp on Function properties → CacheIR: IC for function.length
https://hg.mozilla.org/mozilla-central/rev/a6ed4bb887b1
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
I checked and I'm seeing 37-40ms spent in the function that call function.length so my setup is not sensitive enough to pick up an improvement.
Flags: needinfo?(b56girard)
You need to log in before you can comment on or make changes to this bug.