Improve performance on some post-minifier code. #1682

Penguinwizzard · 2016-10-03T18:31:46Z

Running a minifier on some common benchmarks revealed areas where we could improve our performance.

msftclas · 2016-10-03T18:31:49Z

Hi @Penguinwizzard, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

It looks like you're a Microsoft contributor (Derek Morris). If you're full-time, we DON'T require a Contribution License Agreement. If you are a vendor, please DO sign the electronic Contribution License Agreement. It will take 2 minutes and there's no faxing! https://cla.microsoft.com.

TTYL, MSBOT;

agarwal-sandeep · 2016-10-03T18:47:15Z

lib/Backend/Lower.cpp

                        this->LowerBrCMem(instr, IR::HelperOp_Equal, false);
                    }
                }
+                else if (this->GenerateFastBrEqBoolInt(instr->AsBranchInstr(), &needHelper))


GenerateFastBrEqBoolInt [](start = 31, length = 23)

Should we also have this for CmEq_A and CmNeq_A as most of the code is reusable?

It's definitely doable; I'll take a swing at it, hopefully it also proves effective.

rajatd · 2016-10-03T18:56:21Z

What kind of perf improvements did you see?

Penguinwizzard · 2016-10-04T23:59:44Z

The score of post-minifier octane navier-stokes.js improved ~30%, post-minifier mandreel.js improved ~22%, post-minifier splay.js improved ~24%, and a number of other tests posted smaller improvements. This doesn't impact the score on a non-minified version of octane though.

Penguinwizzard · 2016-10-05T20:15:53Z

@dotnet-bot test Ubuntu ubuntu_linux_debug_static please
@dotnet-bot test Ubuntu ubuntu_linux_test please

rajatd · 2016-10-06T06:17:04Z

lib/Backend/Lower.cpp

+                typeOpnd = instrSrc2;
+                idOpnd = instrSrc1;
+            }
+            if (typeOpnd && idOpnd)


null check for instrSrc1 and instrSrc2 above makes this check redundant.

rajatd · 2016-10-10T23:09:15Z

@pleath @akroshg for bytecode changes

rajatd · 2016-10-10T23:10:25Z

lib/Backend/GlobOpt.cpp

-            || (src2 &&  src2->IsConstOpnd()))
-            && bInstr->GetSrc1()->IsRegOpnd())
+        // These are used because we don't want to rely on src1 or src2 to always be the register/constant
+        IR::RegOpnd *regopnd = nullptr;


regopnd [](start = 21, length = 7)

nit: please use camelCase for variable names.

rajatd · 2016-10-10T23:12:06Z

lib/Backend/GlobOpt.cpp

+        // These are used because we don't want to rely on src1 or src2 to always be the register/constant
+        IR::RegOpnd *regopnd = nullptr;
+        IR::Opnd *constopnd = nullptr;
+        if (!src2 && (instr->m_opcode == Js::OpCode::BrFalse_A || instr->m_opcode == Js::OpCode::BrTrue_A) && src1->IsRegOpnd()) {


nit: opening brace on the next line, as has been done in the rest of the file

rajatd · 2016-10-10T23:14:00Z

lib/Backend/GlobOpt.cpp

-            && bInstr->GetSrc1()->IsRegOpnd())
+        // These are used because we don't want to rely on src1 or src2 to always be the register/constant
+        IR::RegOpnd *regopnd = nullptr;
+        IR::Opnd *constopnd = nullptr;


constopnd [](start = 18, length = 9)

what is constOpnd used for?

I've removed that in the new revision, thanks; it was an artefact of how I ambidextrized the check.

LouisLaf · 2016-11-04T19:18:55Z

lib/Backend/BackwardPass.cpp

+        // part of the block prefix, and put the bailonnoprofile immediately after them. This has the added benefit
+        // that we can still merge up blocks beginning with bailonnoprofile, even if they would otherwise not allow
+        // us to, due to the fact that these tmp declarations would be pre-empted by the higher-level bailout.
+        while (curNext->m_opcode == Js::OpCode::Ld_A && curNext->GetDst()->IsRegOpnd() && curNext->GetDst()->AsRegOpnd()->m_fgPeepTmp)


This can be cleaned up a bit it seems. The while condition is the same as the if, making the if unnecessary.

I've restructured the code to be a single while loop, removing the extra copy of the condition.

LouisLaf · 2016-11-04T19:20:16Z

lib/Backend/BackwardPass.cpp


    // Move to top of block.
-    while(!curInstr->StartsBasicBlock())
+    while(!curInstr->StartsBasicBlock() && curInstr != instrNope)


nstrNope [](start = 56, length = 8)

The way I read this, if instrNope is set, it will always be == curInstr...

Tracing the code seems to indicate that it has the desired effect; while they will be equal in cases where the BailOnNoProfile is already just under the Ld_A fgPeepTmps, in many cases there will be other instructions in between. In that case, instrNope serves as a second "start of block", at which the search back towards the start of the block should stop.

rajatd · 2016-11-11T19:46:55Z

lib/Backend/FlowGraph.h

    uint8                hasCall:1;
    uint8                isVisited:1;
    uint8                isAirLockCompensationBlock:1;
+    uint8                beginsBailOnNoProfile:1;


beginsBailOnNoProfile [](start = 25, length = 21)

nit: consider changing the name to beginsWithBailOnNoProfile

rajatd · 2016-11-11T20:01:24Z

lib/Backend/GlobOpt.cpp

+            ValueInfo *src1ValInfo = src1Val->GetValueInfo();
+            ValueInfo *src2ValInfo = src2Val->GetValueInfo();
+            if (
+                (src1ValInfo->IsNumber() && src1Var && src2ValInfo->IsBoolean() && src1Var != Js::TaggedInt::ToVarUnchecked(0) && src1Var != Js::TaggedInt::ToVarUnchecked(1)) ||


Can we also fold a branch when we get a non-null constant var for the src which is definitely boolean and then compare the constant boolean with the constant number value of the other src?

rajatd · 2016-11-11T20:04:42Z

lib/Backend/GlobOpt.cpp

+            ValueInfo *src2ValInfo = src2Val->GetValueInfo();
+            if (
+                (src1ValInfo->IsNumber() && src1Var && src2ValInfo->IsBoolean() && src1Var != Js::TaggedInt::ToVarUnchecked(0) && src1Var != Js::TaggedInt::ToVarUnchecked(1)) ||
+                (src2ValInfo->IsNumber() && src2Var && src1ValInfo->IsBoolean() && src2Var != Js::TaggedInt::ToVarUnchecked(0) && src2Var != Js::TaggedInt::ToVarUnchecked(1))


nit: I would refactor this condition (and the one above for BrEq_A) into a method.

rajatd · 2016-11-11T20:08:45Z

lib/Backend/Lower.cpp

+        // This opcode is not one of the ones that should be handled here.
+        return false;
+        break;
+    }


Could make this part cleaner. Example,
switch (instr->m_opcode)
{
case Js::OpCode::BrSrEq_A:
case Js::OpCode::BrSrNotNeq_A:
case Js::OpCode::CmSrEq_A:
isStrict = true;
break;

case Js::OpCode::BrSrNeq_A: case Js::OpCode::BrSrNotEq_A: case Js::OpCode::CmSrNeq_A: isStrict = true; case Js::OpCode::BrNeq_A: case Js::OpCode::BrNotEq_A: case Js::OpCode::CmNeq_A: isNeqOp = true; break; }

And I would put an assert in the default case for unexpected opcode.

rajatd · 2016-11-11T20:13:22Z

lib/Backend/Lower.cpp

+        return false;
+    }
+
+    // If either instruction is constant, we can simplify the check. If both are constant, we can eliminate it


instruction [](start = 17, length = 11)

nit: source

rajatd · 2016-11-11T20:23:10Z

lib/Backend/Lower.cpp

+    if (*pNeedHelper)
+    {
+        instr->InsertBefore(labelHelper);
+    }


I would rather have a wrapper for this function, have that wrapper pass the helper label to this function, insert the helper label when this function returns, and replace all callsites of this function with that helper. It would make using this function in other places easier.

rajatd · 2016-11-11T23:47:15Z

lib/Backend/Lower.cpp

+        {
+            srcIntConstVal = true;
+            srcIntIsBoolable = true;
+        }


This block shouldn't be required after @meg-gupta 's #1630

rajatd · 2016-11-11T23:49:06Z

lib/Backend/Lower.cpp

+        }
+        int32 value = Js::TaggedInt::ToInt32(addrSrcBool->m_address);
+        srcBoolConst = true;
+        srcBoolConstVal = value != 0;


This block shouldn't be required after @meg-gupta 's #1630

rajatd · 2016-11-11T23:49:17Z

lib/Backend/Lower.cpp

+    }
+    if (srcBool->IsIntConstOpnd())
+    {
+        Assert(srcBool->IsIntConstOpnd());


Assert(srcBool->IsIntConstOpnd()); [](start = 8, length = 34)

redundant?

rajatd · 2016-11-11T23:52:41Z

lib/Backend/Lower.cpp

+    else if (srcIntConst && srcBoolConst)
+    {
+        // If both arguments are constant, we can statically determine the result.
+        bool sameval = srcIntConstVal == srcBoolConstVal;


sameval [](start = 13, length = 7)

nit: camel case

rajatd · 2016-11-12T00:23:13Z

lib/Backend/Lower.cpp

+    else if (!srcIntConst && !srcBoolConst)
+    {
+        // If neither is constant, we can still do a bit better than loading the helper
+        IR::LabelInstr * firstTrue = IR::LabelInstr::New(Js::OpCode::Label, this->m_func);


firstTrue [](start = 25, length = 9)

no code seems to be branching to this label

msftclas added the cla-not-required label Oct 3, 2016

agarwal-sandeep reviewed Oct 3, 2016

View reviewed changes

Penguinwizzard force-pushed the closure_compiler branch from d10aca7 to c790720 Compare October 5, 2016 17:46

rajatd reviewed Oct 6, 2016

View reviewed changes

rajatd reviewed Oct 10, 2016

View reviewed changes

LouisLaf reviewed Nov 4, 2016

View reviewed changes

Penguinwizzard force-pushed the closure_compiler branch 4 times, most recently from f4a53b1 to 2ae533b Compare November 9, 2016 22:35

rajatd reviewed Nov 11, 2016

View reviewed changes

rajatd reviewed Nov 12, 2016

View reviewed changes

Penguinwizzard force-pushed the closure_compiler branch from 2ae533b to 5a44314 Compare November 12, 2016 02:20

Penguinwizzard force-pushed the closure_compiler branch from 5a44314 to 211ee15 Compare November 12, 2016 02:21

Penguinwizzard closed this Nov 14, 2016

Penguinwizzard force-pushed the closure_compiler branch from 211ee15 to add6acf Compare November 14, 2016 20:04

snyk-bot mentioned this pull request Nov 18, 2020

[Snyk] Security upgrade yargs from 4.8.1 to 16.0.0 saurabharch/ChakraCore#3

Open

Improve performance on some post-minifier code. #1682

Improve performance on some post-minifier code. #1682

Uh oh!

Conversation

Penguinwizzard commented Oct 3, 2016

Uh oh!

msftclas commented Oct 3, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajatd commented Oct 3, 2016

Uh oh!

Penguinwizzard commented Oct 4, 2016

Uh oh!

Penguinwizzard commented Oct 5, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajatd commented Oct 10, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants