Skip to content

Commit 0796760

Browse files
author
Thomas Moore
committed
OS#13129251: Math.min/max should return an integer value when all of its params are integers
This change addresses a perf issue where non-inlined Math.min/max are always floating-point vars. In the bug, this causes expensive bailouts in a loop that was setting to a Uin8ClampedArray. The fix is to check whether all of the parameters are tagged integers, and, if so, return an int. With a reduced repro of the scenario from the original page, there is a significant improvement, where the same function takes 20% of the time it did before. Normal usage of Math.max with 3 int parameters set to a var results in taking 65% of the time it did before. Normal usage of Math.max with a float parameter showed a 1-5% regression, depending on where the first non-int parameter is listed.
1 parent bb6d734 commit 0796760

3 files changed

Lines changed: 88 additions & 28 deletions

File tree

‎lib/Runtime/Language/TaggedInt.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@ namespace Js {
4444
static bool Is(Var aValue);
4545
static bool Is(intptr_t aValue);
4646
static bool IsPair(Var aLeft, Var aRight);
47+
static bool OnlyContainsTaggedInt(Js::Arguments& args);
4748
static double ToDouble(Var aValue);
4849
static int32 ToInt32(Var aValue);
4950
static int32 ToInt32(intptr_t aValue);

‎lib/Runtime/Language/TaggedInt.inl‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -206,7 +206,22 @@ namespace Js
206206
}
207207
#endif
208208

209+
// Returns true when the provided args only contains TaggedInts. Note that
210+
// iteration starts from 1 (to account for 'this' at index 0).
211+
bool inline TaggedInt::OnlyContainsTaggedInt(Js::Arguments& args)
212+
{
213+
bool onlyContainsInts = true;
214+
for (uint idxArg = 1; idxArg < args.Info.Count; idxArg++)
215+
{
216+
if (!TaggedInt::Is(args[idxArg]))
217+
{
218+
onlyContainsInts = false;
219+
break;
220+
}
221+
}
209222

223+
return onlyContainsInts;
224+
}
210225

211226
inline Var TaggedInt::Add(Var aLeft,Var aRight,ScriptContext* scriptContext)
212227
#ifdef DBG

‎lib/Runtime/Library/MathLibrary.cpp‎

Lines changed: 72 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -702,6 +702,7 @@ namespace Js
702702
ARGUMENTS(args, callInfo);
703703
AssertMsg(args.Info.Count > 0, "Should always have implicit 'this'");
704704
ScriptContext* scriptContext = function->GetScriptContext();
705+
bool hasOnlyIntegerArgs = false;
705706

706707
Assert(!(callInfo.Flags & CallFlags_New));
707708

@@ -714,35 +715,56 @@ namespace Js
714715
double result = JavascriptConversion::ToNumber(args[1], scriptContext);
715716
return JavascriptNumber::ToVarNoCheck(result, scriptContext);
716717
}
717-
else if (args.Info.Count == 3)
718+
else
718719
{
719-
if (TaggedInt::Is(args[1]) && TaggedInt::Is(args[2]))
720+
hasOnlyIntegerArgs = TaggedInt::OnlyContainsTaggedInt(args);
721+
if (hasOnlyIntegerArgs && args.Info.Count == 3)
720722
{
721723
return TaggedInt::ToVarUnchecked(max(TaggedInt::ToInt32(args[1]), TaggedInt::ToInt32(args[2])));
722724
}
723725
}
724726

725-
double current = JavascriptConversion::ToNumber(args[1], scriptContext);
726-
if(JavascriptNumber::IsNan(current))
727+
if (hasOnlyIntegerArgs)
727728
{
728-
return scriptContext->GetLibrary()->GetNaN();
729-
}
729+
int32 current = TaggedInt::ToInt32(args[1]);
730+
for (uint idxArg = 2; idxArg < args.Info.Count; idxArg++)
731+
{
732+
int32 compare = TaggedInt::ToInt32(args[idxArg]);
733+
if (current < compare)
734+
{
735+
current = compare;
736+
}
737+
}
730738

731-
for (uint idxArg = 2; idxArg < args.Info.Count; idxArg++)
739+
return TaggedInt::ToVarUnchecked(current);
740+
}
741+
else
732742
{
733-
double compare = JavascriptConversion::ToNumber(args[idxArg], scriptContext);
734-
if(JavascriptNumber::IsNan(compare))
743+
double current = JavascriptConversion::ToNumber(args[1], scriptContext);
744+
if (JavascriptNumber::IsNan(current))
735745
{
736746
return scriptContext->GetLibrary()->GetNaN();
737747
}
738-
if((JavascriptNumber::IsNegZero(current) && compare == 0) ||
739-
current < compare )
748+
749+
for (uint idxArg = 2; idxArg < args.Info.Count; idxArg++)
740750
{
741-
current = compare;
751+
double compare = JavascriptConversion::ToNumber(args[idxArg], scriptContext);
752+
if (JavascriptNumber::IsNan(compare))
753+
{
754+
return scriptContext->GetLibrary()->GetNaN();
755+
}
756+
757+
// In C++, -0.0f == 0.0f; however, in ES, -0.0f < 0.0f. Thus, use additional library
758+
// call to test this comparison.
759+
if ((compare == 0 && JavascriptNumber::IsNegZero(current)) ||
760+
current < compare)
761+
{
762+
current = compare;
763+
}
742764
}
743-
}
744765

745-
return JavascriptNumber::ToVarNoCheck(current, scriptContext);
766+
return JavascriptNumber::ToVarNoCheck(current, scriptContext);
767+
}
746768
}
747769

748770

@@ -760,6 +782,7 @@ namespace Js
760782
ARGUMENTS(args, callInfo);
761783
AssertMsg(args.Info.Count > 0, "Should always have implicit 'this'");
762784
ScriptContext* scriptContext = function->GetScriptContext();
785+
bool hasOnlyIntegerArgs = false;
763786

764787
Assert(!(callInfo.Flags & CallFlags_New));
765788

@@ -772,35 +795,56 @@ namespace Js
772795
double result = JavascriptConversion::ToNumber(args[1], scriptContext);
773796
return JavascriptNumber::ToVarNoCheck(result, scriptContext);
774797
}
775-
else if (args.Info.Count == 3)
798+
else
776799
{
777-
if (TaggedInt::Is(args[1]) && TaggedInt::Is(args[2]))
800+
hasOnlyIntegerArgs = TaggedInt::OnlyContainsTaggedInt(args);
801+
if (hasOnlyIntegerArgs && args.Info.Count == 3)
778802
{
779803
return TaggedInt::ToVarUnchecked(min(TaggedInt::ToInt32(args[1]), TaggedInt::ToInt32(args[2])));
780804
}
781805
}
782806

783-
double current = JavascriptConversion::ToNumber(args[1], scriptContext);
784-
if(JavascriptNumber::IsNan(current))
807+
if (hasOnlyIntegerArgs)
785808
{
786-
return scriptContext->GetLibrary()->GetNaN();
787-
}
809+
int32 current = TaggedInt::ToInt32(args[1]);
810+
for (uint idxArg = 2; idxArg < args.Info.Count; idxArg++)
811+
{
812+
int32 compare = TaggedInt::ToInt32(args[idxArg]);
813+
if (current > compare)
814+
{
815+
current = compare;
816+
}
817+
}
788818

789-
for (uint idxArg = 2; idxArg < args.Info.Count; idxArg++)
819+
return TaggedInt::ToVarUnchecked(current);
820+
}
821+
else
790822
{
791-
double compare = JavascriptConversion::ToNumber(args[idxArg], scriptContext);
792-
if(JavascriptNumber::IsNan(compare))
823+
double current = JavascriptConversion::ToNumber(args[1], scriptContext);
824+
if (JavascriptNumber::IsNan(current))
793825
{
794826
return scriptContext->GetLibrary()->GetNaN();
795827
}
796-
if((JavascriptNumber::IsNegZero(compare) && current == 0) ||
797-
current > compare )
828+
829+
for (uint idxArg = 2; idxArg < args.Info.Count; idxArg++)
798830
{
799-
current = compare;
831+
double compare = JavascriptConversion::ToNumber(args[idxArg], scriptContext);
832+
if (JavascriptNumber::IsNan(compare))
833+
{
834+
return scriptContext->GetLibrary()->GetNaN();
835+
}
836+
837+
// In C++, -0.0f == 0.0f; however, in ES, -0.0f < 0.0f. Thus, use additional library
838+
// call to test this comparison.
839+
if ((current == 0 && JavascriptNumber::IsNegZero(compare)) ||
840+
current > compare)
841+
{
842+
current = compare;
843+
}
800844
}
801-
}
802845

803-
return JavascriptNumber::ToVarNoCheck(current, scriptContext);
846+
return JavascriptNumber::ToVarNoCheck(current, scriptContext);
847+
}
804848
}
805849

806850

0 commit comments

Comments
 (0)