Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid the use of Linq #939

Closed
shargon opened this issue Jul 19, 2019 · 14 comments
Closed

Avoid the use of Linq #939

shargon opened this issue Jul 19, 2019 · 14 comments
Labels
Discussion Initial issue state - proposed but not yet accepted Enhancement Type - Changes that may affect performance, usability or add new features to existing modules. Housekeeping Small enhancements that need to be done in order to keep the project organised

Comments

@shargon
Copy link
Member

shargon commented Jul 19, 2019

I understand that Linq makes our life easier (I ❤️ Linq), but the speed cost is very high, i will put an example here of the difference with the same logic

Code:

        int iterations = 100_000;
        List<int> list = new List<int>(new int[1000]);

        [TestMethod]
        public void WithoutLinq()
        {
            list[list.Count - 1] = 1;

            int yes = 0;
            for (int x = 0; x < iterations; x++)
            {
                foreach(var y in list)
                {
                    if (y == 1)
                    {
                        yes++;
                        break;
                    }
                }
            }

            Assert.AreEqual(iterations, yes);
        }

        [TestMethod]
        public void WithLinq()
        {
            list[list.Count - 1] = 1;

            int yes = 0;
            for (int x = 0; x < iterations; x++)
            {
                if (list.Any(y => y == 1))
                {
                    yes++;
                }
            }

            Assert.AreEqual(iterations, yes);
        }

Results in my computer:

WithLinq: 944 ms
WithoutLinq: 382 ms

We should try to avoid the use of Linq in order to get more TPS

@shargon
Copy link
Member Author

shargon commented Jul 19, 2019

Now with the first entry as 1

        int iterations = 100_000;
        List<int> list = new List<int>(new int[1000]);

        [TestMethod]
        public void WithoutLinq()
        {
            list[0] = 1;

            int yes = 0;
            for (int x = 0; x < iterations; x++)
            {
                foreach(var y in list)
                {
                    if (y == 1)
                    {
                        yes++;
                        break;
                    }
                }
            }

            Assert.AreEqual(iterations, yes);
        }

        [TestMethod]
        public void WithLinq()
        {
            list[0] = 1;

            int yes = 0;
            for (int x = 0; x < iterations; x++)
            {
                if (list.Any(y => y == 1))
                {
                    yes++;
                }
            }

            Assert.AreEqual(iterations, yes);
        }

Results in my computer:

WithLinq: 13 ms
WithoutLinq: 2 ms

@igormcoelho
Copy link
Contributor

Man, this is crazy.. is it Release code?

@shargon
Copy link
Member Author

shargon commented Jul 19, 2019

The previous result was under debug, but these are the result under release and executed alone:

Last entry:

WithLinq: 923 ms
WithoutLinq: 240 ms

First entry:

WithLinq: 13 ms
WithoutLinq: 8 ms

@igormcoelho
Copy link
Contributor

igormcoelho commented Jul 19, 2019

If you pass a Func<T, bool> to a method, shouldnt it be the same as Linq Any?

Example:

// helper class
bool MyAny(this Enumerable<T> set, Func<T, bool> f){
foreach(v in set)
    if(f(v))
        return true;
return false;

}

// test

    if (list.MyAny(y => y == 1))
                {
                    yes++;
                }

Could you try this please shargon?

@lock9 lock9 added the Discussion Initial issue state - proposed but not yet accepted label Jul 20, 2019
@lock9 lock9 added the Enhancement Type - Changes that may affect performance, usability or add new features to existing modules. label Jul 29, 2019
@lock9 lock9 added the Housekeeping Small enhancements that need to be done in order to keep the project organised label Aug 9, 2019
@belane
Copy link
Member

belane commented Aug 14, 2019

@shargon, I understand that this type of optimization is more important in certain parts, for example listing the addresses of a wallet is not as important as the consensus .

Can we start by listing the code where this optimization would have more impact?

@shargon
Copy link
Member Author

shargon commented Aug 14, 2019

Agree, i will work on this in a future, in certains parts are better the readability

@shargon shargon added the Low-Priority Issues with lower priority label Aug 14, 2019
@lock9
Copy link
Contributor

lock9 commented Aug 14, 2019

@shargon I would like to eliminate the low-priority tag, it does not help us. I would like to make prioritization 'per release' instead of per issue.
Can I remove this tag?
What is important is tagged with the next release version, "everything else is 'low priority'".

@shargon
Copy link
Member Author

shargon commented Aug 15, 2019

Of course!

@rodoufu
Copy link

rodoufu commented Sep 5, 2019

@shargon have you ran some performance test on the code (I'm not criticizing, I'm asking cause I haven't)?
Sometimes it's easier to do like that to get better improvements in the beginning. And I'm not talking about lazy coding but sometimes, for the sake of readability (as you've already said) it's better to pay a little, and sometimes it does not make that difference.

I can remember that once I was talking with a colleague about the possibility of using & instead of % to check if a number is odd or even but it was much more ugly and people need much more time to understand it.
Like:

if (a & 1 == 0) {
   // even
}
if (a % 2 == 0) {
   // even
}

@shargon
Copy link
Member Author

shargon commented Sep 5, 2019

I really think that in a future optimization phase "Avoid Linq" is something to keep in mind, just for critical methods. Currently I haven't got time to do benchmarks and I created this issue based on my experience in the past, so if you are interested in do some benchmarks, just do it. But i think that this could be a low priority issue

@vncoelho
Copy link
Member

Before proceeding Igor's question: #939 (comment)

and @rodoufu should be taken into account.

@eryeer
Copy link
Contributor

eryeer commented Nov 11, 2019

@shargon Great work! Think radically, we may need to replace all linq statements, at least in terms of affecting TPS, we need to do this, such as all linq statements about transaction processing.

@shargon
Copy link
Member Author

shargon commented Nov 11, 2019

Agree @eryeer Please review this, #1208 i removed all the linq calls in ECDSA, and the speed up was increased, feel free to do your benchmarks

Thacryba pushed a commit to simplitech/neo that referenced this issue Feb 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion Initial issue state - proposed but not yet accepted Enhancement Type - Changes that may affect performance, usability or add new features to existing modules. Housekeeping Small enhancements that need to be done in order to keep the project organised
Projects
None yet
Development

No branches or pull requests

8 participants