IonMonkey: increase speed of LCallConstructing (i.e. unknown function target) using IM to IM fastpath

RESOLVED FIXED in mozilla20

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: h4writer, Assigned: h4writer)

Tracking

unspecified
mozilla20
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
IM is really bad at the following testcase. The problem here is that we don't inline the "new TEst()" call and have to go through the interpreter to run it. That's the observed overhead between IM and JM. 

Reason: getPolyCallTargets returns 0, because the TypeSet has 1 objCount, that is NULL.

// IM: 2.042s
// JM: 0.778s
// D8: 0.073s

var Class = {
  create: function() {
    return function() {
    }
  }
};

var TEst = Class.create();

for (var i=0; i<10000000; i++) {
  new TEst()
}
(Assignee)

Updated

5 years ago
Blocks: 768745
I thought we were cloning the function such as we can have uniq typesets, but having one object which is set to NULL seems kind of weird.  It sounds like the function has been cloned but that we are calling one is not the one used by the interpreter.
(Assignee)

Comment 2

5 years ago
So the function indeed get's cloned, but doesn't get an unique type.

If the returned function would have "this.initialize.apply(this, arguments);", like in bug #813784, we are smart enough to create new types, because of "UseNewTypeForClone()" in jsinferinlines.h:676 and have a SingleObject.
Here we use LCallKnown

In the non-singleObject case, we don't have a target and we call LCallConstructor. E.g. resulting in the slowdown.
(Assignee)

Comment 3

5 years ago
JM does also invoke interpreter, but can afterwards patch it:

[jaeger] PICs     stop ignored: first hit (reduced/raytrace/test1.js: 6)
[jaeger] PICs     getprop patch: inline (/reduced/raytrace/test1.js: 6)
[jaeger] PICs     patched CALL path 0x7fc023561370 (obj: 0x7fc021a19fc0)

The last patching makes the calling as fast as if the target was known
(with an extra check if the object is correct)

In order to get IM as fast as JM in this case, we will have to do that too
(Assignee)

Comment 4

5 years ago
Created attachment 685155 [details] [diff] [review]
Use CallGeneric for constructor functions

For constructing function we currently always take a VM call. That's why we are so much slower. LCallGeneric is much smarter than LCallConstructor. It was easier to remove LCallConstructor and make LCallGeneric to work with constructing functions. Therefore LCallConstructor is removed in this patch.

python jit_test.py --ion $JS/js
PASSED ALL

time $JS/js test.js
1.027s

An increase of 2x on this small testcase. We are still 0.2s slower than JM. I suspect because of the tricks JM applies. E.g. the getprop (I assume the prototype getprop) gets cached. In IM not, because we use "MCallGetProperty".

Another note is that the timing with/without constructing is the same for V8! In our engine there is a huge difference with/without constructing. Also our non-constructing speed is almost the same on this benchmark, it is only our constructing speed that is terrible slower!
Assignee: general → hv1989
(Assignee)

Updated

5 years ago
Attachment #685155 - Flags: review?(npierron)
Why is our constructing performance so much worse?
Status: NEW → ASSIGNED
Comment on attachment 685155 [details] [diff] [review]
Use CallGeneric for constructor functions

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

This sounds good to me, add the comment and r=me.
I am adding sstangl in case he has any remarks about the removal of LCallConstructor.

As mentionned on IRC, this does not fix the root of the problem which is that TI does not provide a signle type-object after the function is cloned, but this patch still improve by sharing more code and optimize by creating a New this by looking at the prototype of the callee.

::: js/src/ion/CodeGenerator.cpp
@@ +904,5 @@
> +
> +    if (call->mir()->isConstructing()) {
> +        Label notPrimitive;
> +        masm.branchTestPrimitive(Assembler::NotEqual, JSReturnOperand, &notPrimitive);
> +        masm.loadValue(Address(StackPointer, unusedStack), JSReturnOperand);

Can you comment what we are reading from the stack and why we erase the return value?
Attachment #685155 - Flags: review?(sstangl)
Attachment #685155 - Flags: review?(npierron)
Attachment #685155 - Flags: review+
Comment on attachment 685155 [details] [diff] [review]
Use CallGeneric for constructor functions

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

This seems fine. Missing the constructing StackFrame flag may affect debuggers, though.

::: js/src/ion/CodeGenerator.cpp
@@ +904,5 @@
> +
> +    if (call->mir()->isConstructing()) {
> +        Label notPrimitive;
> +        masm.branchTestPrimitive(Assembler::NotEqual, JSReturnOperand, &notPrimitive);
> +        masm.loadValue(Address(StackPointer, unusedStack), JSReturnOperand);

I remember this code existing multiple times in LCallKnown. Can it be reused?

::: js/src/ion/IonBuilder.cpp
@@ +3571,5 @@
>  
>  MDefinition *
>  IonBuilder::createThis(HandleFunction target, MDefinition *callee)
>  {
> +    if (target) {

It would look nicer if we handled the !target case up-top, to remove an indentation level from the bulk of this function, even at the cost of repeating createThisScripted(). I would prefer that this function be left as it was before, with the addition of the !target case.

@@ +3791,5 @@
>  
>      MPassArg *thisArg = current->pop()->toPassArg();
>  
>      // If the target is known, inline the constructor on the caller-side.
> +    if (constructing) {

The comment above is now incorrect.
Attachment #685155 - Flags: review?(sstangl) → review+

Comment 9

5 years ago
https://hg.mozilla.org/mozilla-central/rev/5158d648702e
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Backed out
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b4e13b0d1e4

See bug 16378
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 11

5 years ago
(In reply to Hub Figuiere [:hub] from comment #10)
> See bug 816378

Updated

5 years ago
Depends on: 816378
(Assignee)

Updated

5 years ago
Depends on: 816492
(Assignee)

Updated

5 years ago
Summary: IonMonkey: IM to IM calls fails on reduced raytrace testcase → IonMonkey: increase speed of LCallConstructing (i.e. unknown function target) using IM to IM fastpath
(Assignee)

Updated

5 years ago
Depends on: 819299
(Assignee)

Comment 14

5 years ago
This was backed out again, This time because #819299 got backed out for failing on 32bit architectures. Will recommit when other patch gets committed again.

64bit just worked and therefore I have already numbers:
- 0.3% increase on kraken, 
-- 1.1% kraken-astar
-- 1.6% kraken-crypto-ccm
-- 1.4% kraken-crypto-pbkdf2
-- 2.6% kraken-parse-financial
- 0.4% increase on V8
-- 2.8% increase on v8-richards
https://hg.mozilla.org/mozilla-central/rev/32a37874bfd9
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Blocks: 816462

Updated

5 years ago
Depends on: 825705
You need to log in before you can comment on or make changes to this bug.