BigInt: reallocate space, digit_t, increment, decrement #5788

duongnhn · 2018-10-17T23:50:56Z

In this PR, we implement

reallocate space for BigInt if necessary -> make it arbitrarily-precision.
make use of configuration with digit_t
implement increment/decrement operators
native tests for methods in Javascript BigInt class

I defer "assign along with inc/dec" y=x++ and y=++x to a future PR

duongnhn · 2018-10-18T00:03:41Z

lib/Common/Core/CommonTypedefs.h

@@ -61,3 +61,6 @@ namespace Js
 {
    typedef uint32 LocalFunctionId;
 };
+
+// digit_t represents a digit in bigint underline
+typedef uintptr_t digit_t;


define a digit of BigInt

sethbrenith · 2018-10-18T20:23:22Z

test/BigInt/increment.js

+        body: function () {
+            var x = 123n;
+            assert.isTrue(x == 123n);
+            x++;


Perhaps we could also capture the result of these expressions, to verify prefix versus postfix behavior. Something like this:

assert.isTrue(x == 123n); var y = x++; assert.isTrue(x == 124n); assert.isTrue(y == 123n); y = ++x; assert.isTrue(x == 125n); assert.isTrue(y == 125n);

(Certainly not necessary in each test case, and if this is already adequately covered by test262 feel free to ignore.)

that's is what I did in assign_by_value.js

Sorry, I don't think I explained very well what I was hoping to see. I thought it might be nice to have a test that uses the result of the increment expression itself, like an assignment or passing it to a method. I see a lot of x++; statements on their own, but never a y = x++ or a f(x++).

In reply to: 226483957 [](ancestors = 226483957)

Got you but I don't think this PR can address assignment with pre/post inc/dec. This is in my TODO lists

I expect that if you start AbsoluteIncrement and AbsoluteDecrement with cloning the existing BigInt, then everything else should work out correctly.

In reply to: 226508075 [](ancestors = 226508075)

@sethbrenith I think it is more to that (tried, didn't work). Also, when we do y=x++, the existing flow do convert x to number which can not apply directly to bigint.

sethbrenith · 2018-10-18T20:25:48Z

lib/Runtime/Library/JavascriptBigInt.inl

@@ -0,0 +1,29 @@
+//-------------------------------------------------------------------------------------------------------


How did this file come back? Maybe a merge issue? These shouldn't be necessary anymore because you defined VarIsImpl.

will delete. this branch is created a long back :)

sethbrenith · 2018-10-18T20:33:10Z

lib/Runtime/Language/JavascriptOperators.cpp

@@ -10627,7 +10627,7 @@ using namespace Js;
    Var JavascriptOperators::ToNumber(Var aRight, ScriptContext* scriptContext)


Is this the abstract operation ToNumber as defined in the spec? If so it should throw a TypeError if called on a BigInt.

sethbrenith · 2018-10-18T20:34:58Z

lib/Runtime/Language/ProfilingHelpers.cpp

@@ -994,6 +994,10 @@ using namespace Js;
                        : PatchGetMethod ;
                value = (!Root && !Method) ? PatchGetValue(functionBody, inlineCache, inlineCacheIndex, object, propertyId, thisObject) :
                    PatchGet(functionBody, inlineCache, inlineCacheIndex, object, propertyId);
+                if (VarIs<JavascriptBigInt>(value))


I'm surprised that this type would need special handling here. If you override the instance method CloneToScriptContext inherited from RecyclableObject, does it maybe just work?

roll it back and defer to a future PR

sethbrenith · 2018-10-18T20:51:46Z

lib/Runtime/Library/JavascriptBigInt.cpp

+        bigintNew->m_isNegative = pbi->m_isNegative;
+        bigintNew->m_digits = bigintNew->m_reservedDigitsInit;
+        bigintNew->Resize(bigintNew->m_length);
+        for (digit_t i = 0; i < bigintNew->m_length; i++)


nit: could use memcpy for this step (the compiler might be clever enough to emit something similar based on this loop, I'm not sure)

sethbrenith · 2018-10-18T20:55:53Z

test/BigInt/comparison.js

@@ -72,6 +72,19 @@ var tests = [
            assert.isFalse(x > y);
        }
    },
+    {
+        name: "Out of 64 bit range",


Would be nice to have some tests with ridiculously huge numbers so they exercise your dynamic allocation code. You can use strings, the .repeat method, and eval to avoid typing the numbers in directly: just generate a string with a big literal and eval it.

sethbrenith · 2018-10-18T21:01:19Z

lib/Runtime/Library/JavascriptBigInt.h

-        static const uint32 MaxStringLength = 100; // Max String length
-        static const uint32 MaxDigitLength = 20;  // Max Digit length 
-        Field(uint32) m_reservedDigitsInit[MaxDigitLength]; // pre-defined space to store array
+        static const digit_t InitDigitLength = 20;  // Max Digit length 


This is rather a lot of space to allocate by default. BigInt is pretty new and not a whole lot of code uses it yet, but I would guess that the majority of BigInts fit within 2 digits.

let it be 2 then 👍

sethbrenith · 2018-10-18T21:11:01Z

lib/Runtime/Library/JavascriptBigInt.cpp

+        length += length;//double size
+        if (m_digits == m_reservedDigitsInit)
+        {
+            if ((SIZE_MAX / sizeof(digit_t) < length) || (NULL == (digits = (digit_t *)malloc(length * sizeof(digit_t)))))


We should RecyclerNewArrayLeaf rather than malloc/realloc. Then the recycler can manage cleaning up the memory without having to do complicated finalization logic for this class. "Leaf" means the allocation doesn't contain any pointers to other recycler objects, so the recycler doesn't waste time scanning it or risk misinterpreting some plain data as a pointer and keeping stuff alive when it shouldn't.

Also, I'd prefer to have the allocation and assignment in its own statement rather than hidden out halfway through a long conditional.

sethbrenith · 2018-10-18T21:19:12Z

lib/Runtime/Library/JavascriptBigInt.cpp

+
+    JavascriptBigInt * JavascriptBigInt::AbsoluteIncrement(JavascriptBigInt * pbi)
+    {
+        JavascriptBigInt* result = pbi;


Should we make a new one at this point? I expect that in-place changes might end up being useful as a performance optimization, but BigInt is meant to be logically immutable. Consider the following:

x = 123n; y = x; ++x; print(y); // still 123

The ++x means "make a new BigInt literal that is one bigger than x and store it in x", but anybody with pointers to the old one needs to still have the old value.

fixed, I will make a new one before passing into increment/decrement function

sethbrenith · 2018-10-18T21:30:26Z

lib/Runtime/Library/JavascriptBigInt.cpp

+            if (0 < m_length)
+                js_memcpy_s(digits, length * sizeof(digit_t), m_digits, m_length * sizeof(digit_t));
+        }
+        else if (NULL == (digits = (digit_t *)realloc(m_digits, length * sizeof(digit_t))))


Here's a thought: this business of reallocating and copying is great for all kinds of places where you don't know ahead of time how much storage you need. However, it seems to me like we always have a pretty good idea of the upper bound on the number of digits we need to build a BigInt:

Addition, subtraction: Length of the longer input plus 1

Multiplication: Sum of the count of digits in each input

Parsing: Directly related to the input string length, which I think the scanner already gives us (we'd have to peek at the beginning for "0x" etc to figure out the radix).

Anyhow, not a blocker, just a thought.

Yes, it is a great thought. Since reallocate operation is costly then it would save us a lot. This Resize function would be called after we examine the need like your suggesstion.

sethbrenith · 2018-10-18T21:37:25Z

lib/Runtime/Library/JavascriptBigInt.cpp

+
+    Var JavascriptBigInt::Decrement(Var aRight)
+    {
+        AssertMsg(VarIs<JavascriptBigInt>(aRight), "BigInt Increment");


Also, VarTo already asserts this, so feel free to remove if you like.

In reply to: 226474297 [](ancestors = 226474297)

sethbrenith · 2018-10-18T21:42:29Z

lib/Runtime/Library/JavascriptBigInt.cpp

+        //    + [AH*BH]        // rHigh
+        //    = [R1 R2 R3 R4]  // high = [R1 R2], low = [R3 R4]
+        //
+#ifdef TARGET_64


You can avoid the #if stuff by using sizeof. These expressions are compile-time constant so the compiler should do the right thing and emit numbers directly.

digit_t kHalfDigitBits = sizeof(digit_t) * 4; digit_t kHalfDigitMask = (1 << kHalfDigitBits) - 1;

somehow this does not work, failed on native tests

I add one cast to the 2nd line and it seems to work fine

digit_t kHalfDigitBits = sizeof(digit_t) * 4; digit_t kHalfDigitMask = ((digit_t)1 << kHalfDigitBits) - 1;

sethbrenith · 2018-10-18T21:57:46Z

lib/Runtime/Library/JavascriptBigInt.cpp

+    // return low(a*b) and out high
+    digit_t JavascriptBigInt::MulDigit(digit_t a, digit_t b, digit_t* resultHigh)
+    {
+        // Multiply is performed in half chuck favor.


It's good to have a portable implementation of this, but we might find that perf is improved by using platform-specific implementations. For example, on x64 when compiling with MSVC, we could use _mul128.

I will use _umul128 for x64 but cannot find something like_umul64 on win32

it fail on Linux though so I revert back to first implementation. I think we can improve it later if needed

sethbrenith · 2018-10-18T22:05:06Z

lib/Runtime/Language/InterpreterStackFrame.cpp

@@ -8615,6 +8615,10 @@ namespace Js

    Var InterpreterStackFrame::OP_Ld_A(Var aValue)
    {
+        if (VarIs<JavascriptBigInt>(aValue))
+        {
+            return JavascriptBigInt::CloneToScriptContext(aValue, this->GetScriptContext());


Similar to another comment I made elsewhere, this makes me kind of uncomfortable. Here's an operation that was working consistently for every type (even Proxy!) and now it needs special handling for BigInt? I feel like this is a workaround for a more fundamental issue somewhere else.

you are right. I will defer it to a future PR when I find a better approach

sethbrenith · 2018-10-19T15:16:44Z

lib/Runtime/Language/JavascriptOperators.cpp

@@ -299,7 +299,7 @@ using namespace Js;
    Var JavascriptOperators::ToNumberInPlace(Var aRight, ScriptContext* scriptContext, JavascriptNumber* result)


Here too, if this is the spec operation ToNumber, it should throw on BigInt.

will remove

sethbrenith · 2018-10-19T15:19:55Z

lib/Runtime/Library/JavascriptBigInt.h

-        Field(uint32) m_length;          // length
+        Field(digit_t*) m_digits;         // digits
+        Field(digit_t) m_length;          // length
+        Field(digit_t) m_maxLength;          // max length without resize
        Field(bool) m_isNegative;

        // TODO: BigInt should be arbitrary-precision, need to resize space and add tagged BigInt


could remove this TODO since you're doing it :)

will remove

sethbrenith · 2018-10-19T15:34:47Z

lib/Runtime/Library/JavascriptBigInt.cpp

+            return false;
+        }
+
+        digits = RecyclerNewArrayLeaf(scriptContext->GetRecycler(), digit_t, length);


I think you could use GetScriptContext()->GetRecycler() to avoid passing in the ScriptContext. Ideally the only case we would need to pass the script context is in CloneToScriptContext, which is explicitly requesting a new copy of the object in some other script context (not the script context it was created from).

sethbrenith · 2018-10-19T15:37:59Z

lib/Runtime/Library/JavascriptBigInt.h


        inline BOOL isNegative() { return m_isNegative; }

        static JavascriptBigInt * Create(const char16 * content, charcount_t cchUseLength, bool isNegative, ScriptContext * scriptContext);
+        static JavascriptBigInt * CloneToScriptContext(Var aValue, ScriptContext* scriptContext);


Could you please change this signature to RecyclableObject * CloneToScriptContext(ScriptContext* requestContext) override so it overrides the method inherited from RecyclableObject? There aren't any tests for that case yet, but we'll need it eventually.

sethbrenith · 2018-10-19T19:37:43Z

test/BigInt/assign_by_value.js

+            x++;
+            assert.isTrue(x == 124n);
+            assert.isTrue(y == 123n);
+            y = x;


I'm glad to see this test working again :)

yeah, thanks to your suggestion 👍

sethbrenith · 2018-10-19T19:41:16Z

lib/Runtime/Library/JavascriptBigInt.h

+        static bool IsZero(JavascriptBigInt * pbi);
+        static JavascriptBigInt * AbsoluteIncrement(JavascriptBigInt * pbi);
+        static JavascriptBigInt * AbsoluteDecrement(JavascriptBigInt * pbi);
+        static JavascriptBigInt * Increment(JavascriptBigInt * aValue);


Could these four methods return void to make it obvious they are an in-place update? Otherwise somebody glancing at the code might have a hard time understanding the subtle but important difference between Increment(JavascriptBigInt*) and Increment(Var).

sethbrenith

change to digit_t add nativetest for add,sub,mul with digit

duongnhn · 2018-10-22T16:40:37Z

@dotnet-bot test this please

… increment, decrement Merge pull request chakra-core#5788 from duongnhn:user/duongn/bigint_size_t In this PR, we implement - reallocate space for BigInt if necessary -> make it arbitrarily-precision. - make use of configuration with digit_t - implement increment/decrement operators - native tests for methods in Javascript BigInt class I defer "assign along with inc/dec" `y=x++` and `y=++x` to a future PR

duongnhn commented Oct 18, 2018

View reviewed changes

sethbrenith reviewed Oct 18, 2018

View reviewed changes

duongnhn force-pushed the user/duongn/bigint_size_t branch from 2a82748 to 7192fb9 Compare October 18, 2018 23:09

sethbrenith reviewed Oct 19, 2018

View reviewed changes

sethbrenith approved these changes Oct 20, 2018

View reviewed changes

duongnhn force-pushed the user/duongn/bigint_size_t branch 2 times, most recently from 9f8f2b6 to fcb7e88 Compare October 22, 2018 16:14

implement dymamic memory allocation, imcrement and decrement with bigint

88ce20f

change to digit_t add nativetest for add,sub,mul with digit

duongnhn force-pushed the user/duongn/bigint_size_t branch from fcb7e88 to 88ce20f Compare October 22, 2018 16:24

chakrabot merged commit 88ce20f into chakra-core:master Oct 23, 2018

		@@ -0,0 +1,29 @@
		//-------------------------------------------------------------------------------------------------------

		@@ -10627,7 +10627,7 @@ using namespace Js;
		Var JavascriptOperators::ToNumber(Var aRight, ScriptContext* scriptContext)

		@@ -299,7 +299,7 @@ using namespace Js;
		Var JavascriptOperators::ToNumberInPlace(Var aRight, ScriptContext* scriptContext, JavascriptNumber* result)

BigInt: reallocate space, digit_t, increment, decrement #5788

BigInt: reallocate space, digit_t, increment, decrement #5788

Conversation

duongnhn commented Oct 17, 2018 • edited Loading

Choose a reason for hiding this comment

sethbrenith Oct 18, 2018 • edited Loading

Choose a reason for hiding this comment

duongnhn Oct 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duongnhn Oct 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sethbrenith Oct 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sethbrenith Oct 18, 2018 • edited Loading

Choose a reason for hiding this comment

duongnhn Oct 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duongnhn Oct 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duongnhn Oct 19, 2018 • edited Loading

Choose a reason for hiding this comment

duongnhn Oct 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duongnhn Oct 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sethbrenith left a comment

Choose a reason for hiding this comment

duongnhn commented Oct 22, 2018

duongnhn commented Oct 17, 2018 •

edited

Loading

sethbrenith Oct 18, 2018 •

edited

Loading

duongnhn Oct 18, 2018 •

edited

Loading

duongnhn Oct 19, 2018 •

edited

Loading

sethbrenith Oct 18, 2018 •

edited

Loading

sethbrenith Oct 18, 2018 •

edited

Loading

duongnhn Oct 19, 2018 •

edited

Loading

duongnhn Oct 18, 2018 •

edited

Loading

duongnhn Oct 19, 2018 •

edited

Loading

duongnhn Oct 19, 2018 •

edited

Loading

duongnhn Oct 19, 2018 •

edited

Loading