Skip to content
This repository has been archived by the owner on Nov 22, 2023. It is now read-only.

Optimize memory limits #230

Closed
wants to merge 24 commits into from
Closed

Conversation

shargon
Copy link
Member

@shargon shargon commented Nov 23, 2019

Close #202

[TestMethod]
        public void Bench()
        {
            MaxItemTest(false, 1000);
        }

        public void MaxItemTest(bool error, int iterations = 1)
        {
            var items = 1000;
            var script = new ScriptBuilder();

            for (int x = 0; x < iterations; x++)
            {
                script.Emit(OpCode.PUSH0);
                script.Emit(OpCode.NEWARRAY);
                script.Emit(OpCode.TOALTSTACK);

                for (int y = 0; y < (error ? items : items - 1); y++)
                {
                    script.Emit(OpCode.DUPFROMALTSTACK);
                    script.Emit(OpCode.PUSH0);
                    script.Emit(OpCode.APPEND); // Force stack count
                }

                script.Emit(OpCode.FROMALTSTACK);
                script.Emit(OpCode.DROP);
            }

            Stopwatch sw = Stopwatch.StartNew();

            var engine = new ExecutionEngine();
            engine.StackItemMemory.Reserved = items;

            engine.LoadScript(script.ToArray());
            Assert.AreEqual(error ? VMState.FAULT : VMState.HALT, engine.Execute());
            sw.Stop();

            Console.WriteLine(sw.Elapsed);
        }

Results in release mode:

With this patch : 00:00:01.4390325
Before the patch: 00:00:24.2408242

Also we can use a better algorithm, depending the bytes and not the items. using the method AllocateMemory

Pros:

  • Speed up x16.84 FASTER
  • We can put the limit by size, and remove other limits (16mb for example)

Cons:

  • We can't cast List or StackItem[] to Array, we need to create the CompoundType with the ReservedMemory argument.
  • TODO: Do more checks abour circular recursion.

@shargon shargon requested a review from erikzhang November 23, 2019 18:58
@shargon shargon requested a review from longfeiWan9 November 23, 2019 19:40
Copy link
Contributor

@lock9 lock9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great PR @shargon. This is a perfect solution to the problem.
I don't feel confident enough to approve this without proper testing. Are there any special tests you want for this PR?

@shargon
Copy link
Member Author

shargon commented Nov 23, 2019

We need a lot of tests! :)

@erikzhang
Copy link
Member

@shargon Can you briefly explain what the PR did?

@shargon
Copy link
Member Author

shargon commented Nov 24, 2019

The changes aim to unify where the items are allocated, in order to simplify the counter of these items. Previously we had an expensive procedure for checking if we overpass the limits (ussually the StackItems number 2048)

Now, all Random Access Stack has his own Reserved Memory class, also it could be shared between other stacks. Every Time that the stack change his internal list, he will communicate with his Reserved Memory in order to update this information.

The problem comes with Array, Map and Struct, because they can change outside the Stack, so i removed the cast and now you need to create them with the ReservedMemory that they must use, and as with the stacks, every time that his internal list change, he will comunicate with the ReservedMemory

The reserved memory could manage different strategies, now should act as before the change. Because on AllocateMemory we use 1 by default, and the Reserved is the number of items.
But if we want to use the real memory behaviour, we can Allocate the real number of bytes that this StackItems is using GetByteLength when we call AllocateMemory and Reserve 2mb or 4 mb (for example). With this change we can remove other limit like MaxArraySize or MaxItemSize

@erikzhang
Copy link
Member

@shargon We don't need ReservedMemory. We can use reference-tracing. If you agree, I can make a PR.

@shargon
Copy link
Member Author

shargon commented Nov 24, 2019

Let's do it @erikzhang

@erikzhang
Copy link
Member

@shargon Please review #232.

@erikzhang erikzhang mentioned this pull request Nov 28, 2019
@shargon
Copy link
Member Author

shargon commented Nov 29, 2019

Closed in favor of #232

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Optimize the limits inside the VM
3 participants