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

Improve Math.DivRem performance #8125

Merged
merged 2 commits into from Nov 15, 2016

Conversation

Projects
None yet
6 participants
@stephentoub
Member

stephentoub commented Nov 15, 2016

Per the discussion at #3439, this PR at least temporarily changes the implementation of Math.DivRem to avoid two idiv instructions. It then replaces a few % and / pairs in mscorlib with Math.DivRem.

Perf test:

using System;
using System.Diagnostics;

class Program
{
    static long div, rem, a, b;

    static void Main()
    {
        // Various sets of a/b inputs
        var pairs = new[]
        {
            Tuple.Create<long, long>(0, 1),
            Tuple.Create<long, long>(4, 2),
            Tuple.Create<long, long>(99, 10),
            Tuple.Create<long, long>(-99, -10),
            Tuple.Create<long, long>(-99, 10),
            Tuple.Create<long, long>(4, 12),
            Tuple.Create<long, long>(1, long.MaxValue),
            Tuple.Create<long, long>(long.MaxValue, 1),
            Tuple.Create<long, long>(long.MaxValue, 3),
            Tuple.Create<long, long>(long.MaxValue, long.MaxValue - 1),
        };
        const int Iters = 50000000;

        // Time the old and the new on each input
        var sw = new Stopwatch();
        foreach (Tuple<long, long> t in pairs)
        {
            a = t.Item1;
            b = t.Item2;

            sw.Restart();
            for (int i = 0; i < Iters; i++) div = DivRem_Old(a, b, out rem);
            sw.Stop();
            TimeSpan oldTime = sw.Elapsed;

            sw.Restart();
            for (int i = 0; i < Iters; i++) div = DivRem_New(a, b, out rem);
            sw.Stop();
            TimeSpan newTime = sw.Elapsed;

            Console.WriteLine($"Old: {oldTime} New: {newTime}");
        }
    }

    public static long DivRem_Old(long a, long b, out long result)
    {
        result = a % b;
        return a / b;
    }

    public static long DivRem_New(long a, long b, out long result)
    {
        long div = a / b;
        result = a - (div * b);
        return div;
    }
}

outputs for me:

> corerun divremtest.exe
Old: 00:00:01.0659336 New: 00:00:00.5490556
Old: 00:00:01.0559024 New: 00:00:00.5516266
Old: 00:00:01.0675624 New: 00:00:00.5485116
Old: 00:00:01.0537601 New: 00:00:00.5483118
Old: 00:00:01.0612407 New: 00:00:00.5526581
Old: 00:00:01.0874783 New: 00:00:00.5545691
Old: 00:00:01.1375759 New: 00:00:00.5528239
Old: 00:00:01.0875608 New: 00:00:00.5720249
Old: 00:00:01.1346930 New: 00:00:00.6171032
Old: 00:00:01.1621859 New: 00:00:00.5689636

cc: @jkotas, @mikedn, @redknightlois

Improve Math.DivRem performance
Until the JIT is able to eliminate one of the two idiv operations, using a multiplication and subtraction is measurably faster than an extra division.
@redknightlois

This comment has been minimized.

Show comment
Hide comment
@redknightlois

redknightlois Nov 15, 2016

I would aggresively inline it by default if it were my codebase.

redknightlois commented Nov 15, 2016

I would aggresively inline it by default if it were my codebase.

@mikedn

This comment has been minimized.

Show comment
Hide comment
@mikedn

mikedn Nov 15, 2016

Contributor

I'm not sure that using DivRem when the divisor is constant is a good idea. The JIT already handles this partially and I have an open PR that will improve that.

Contributor

mikedn commented Nov 15, 2016

I'm not sure that using DivRem when the divisor is constant is a good idea. The JIT already handles this partially and I have an open PR that will improve that.

@stephentoub

This comment has been minimized.

Show comment
Hide comment
@stephentoub

stephentoub Nov 15, 2016

Member

I'm not sure that using DivRem when the divisor is constant is a good idea

Reverting those

Member

stephentoub commented Nov 15, 2016

I'm not sure that using DivRem when the divisor is constant is a good idea

Reverting those

Use Math.DivRem in another place in mscorlib
Went through all uses of % looking for places DivRem could be used.  This looks like it's the only one of note.
@mikedn

This comment has been minimized.

Show comment
Hide comment
@mikedn

mikedn Nov 15, 2016

Contributor

LGTM

Contributor

mikedn commented Nov 15, 2016

LGTM

@redknightlois

This comment has been minimized.

Show comment
Hide comment
@redknightlois

redknightlois commented Nov 15, 2016

LGTM

@jkotas

jkotas approved these changes Nov 15, 2016

@stephentoub

This comment has been minimized.

Show comment
Hide comment
@stephentoub

stephentoub Nov 15, 2016

Member

@dotnet-bot test Ubuntu x64 Checked Build and Test please (#6574)

Member

stephentoub commented Nov 15, 2016

@dotnet-bot test Ubuntu x64 Checked Build and Test please (#6574)

@stephentoub stephentoub merged commit 68e3e94 into dotnet:master Nov 15, 2016

14 checks passed

CentOS7.1 x64 Debug Build and Test Build finished.
Details
FreeBSD x64 Checked Build Build finished.
Details
Linux ARM Emulator Cross Debug Build Build finished.
Details
Linux ARM Emulator Cross Release Build Build finished.
Details
OSX x64 Checked Build and Test Build finished.
Details
Ubuntu x64 Checked Build and Test Build finished.
Details
Ubuntu x64 Formatting Build finished.
Details
Windows_NT arm Cross Debug Build Build finished.
Details
Windows_NT arm Cross Release Build Build finished.
Details
Windows_NT x64 Debug Build and Test Build finished.
Details
Windows_NT x64 Formatting Build finished.
Details
Windows_NT x64 Release Priority 1 Build and Test Build finished.
Details
Windows_NT x86 legacy_backend Checked Build and Test Build finished.
Details
Windows_NT x86 ryujit Checked Build and Test Build finished.
Details

@stephentoub stephentoub deleted the stephentoub:divrem_perf branch Nov 15, 2016

@karelz karelz modified the milestone: 2.0.0 Aug 28, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment