Closed Bug 813773 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: h4writer, Assigned: h4writer)

References

Details

Attachments

(1 file)

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()
}
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.
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.
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
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
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+
https://hg.mozilla.org/mozilla-central/rev/5158d648702e
Status: ASSIGNED → RESOLVED
Closed: 7 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 → ---
(In reply to Hub Figuiere [:hub] from comment #10)
> See bug 816378
Depends on: 816378
Depends on: 816492
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
Depends on: 819299
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
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Blocks: 816462
Depends on: 825705
You need to log in before you can comment on or make changes to this bug.