Skip to content

Slight optimization for std.conv.text/wtext/dtext#5017

Merged
JackStouffer merged 1 commit intodlang:masterfrom
JackStouffer:textImpl
Jan 13, 2017
Merged

Slight optimization for std.conv.text/wtext/dtext#5017
JackStouffer merged 1 commit intodlang:masterfrom
JackStouffer:textImpl

Conversation

@JackStouffer
Copy link
Contributor

Increase the performance of textImpl by

  • Reducing GC allocations with numbers by using toChars rather than to, which allocates a temporary array and then throws it away
  • Using a very dumb heuristic to pre-allocate space for the elements

Just adding the toChars case gave a small boost in LDC and no difference in DMD. But, the heuristic added a lot of performance.

Recording performance gains of 12.8% with LDC and 5.5% with DMD

$ ldc2 -O5 -release test.d && ./test
result	1 sec, 435 ms, 49 μs, and 4 hnsecs
result2	1 sec, 251 ms, 957 μs, and 6 hnsecs

$ dmd -O -inline -release -noboundscheck test.d &&./test
result	2 secs, 510 ms, 745 μs, and 2 hnsecs
result2	2 secs, 372 ms, 147 μs, and 4 hnsecs

The code

import std.stdio;
import std.range;
import std.algorithm;
import std.traits;
import std.container;
import std.meta;
import std.conv;
import std.datetime;

enum testCount = 1_000_000;

private S textImpl(S, U...)(U args)
{
    static if (U.length == 0)
    {
        return null;
    }
    else static if (U.length == 1)
    {
        return to!S(args[0]);
    }
    else
    {
        import std.array : appender;

        auto app = appender!S();

        foreach (arg; args)
            app.put(arg.to!S);

        return app.data;
    }
}


private S textImpl2(S, U...)(U args)
{
    static if (U.length == 0)
    {
        return null;
    }
    else static if (U.length == 1)
    {
        return to!S(args[0]);
    }
    else
    {
        import std.array : appender;

        auto app = appender!S();

        // assume that on average, parameters will have less
        // than 20 elements
        app.reserve(U.length * 20);

        foreach (arg; args)
        {
            static if (is(Unqual!T == uint) || is(Unqual!T == ulong) || is(Unqual!T == int) || is(Unqual!T == long))
                app.put(arg.toChars);
            else
                app.put(arg.to!S);
        }

        return app.data;
    }
}


void main()
{
    auto result = to!Duration(benchmark!(() => textImpl!dstring(4243, ' ', 1.54321, ": xyz", 333, "Another test"))(testCount)[0]);
    auto result2 = to!Duration(benchmark!(() => textImpl2!dstring(4243, ' ', 1.54321, ": xyz", 333, "Another test"))(testCount)[0]);

    writeln("result", "\t", result);
    writeln("result2", "\t", result2);
}

std/conv.d Outdated
foreach (arg; args)
app.put(to!S(arg));
{
static if (is(Unqual!T == uint) || is(Unqual!T == ulong) || is(Unqual!T == int) || is(Unqual!T == long))
Copy link
Member

@MetaLang MetaLang Jan 5, 2017

Choose a reason for hiding this comment

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

static if (isIntegral!(Unqual!T))?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I copied it from the constraint of toChars

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see. I wonder why toChars defines its constraint to only take u/ints and u/longs... Maybe I'll take a look sometime and see if it can be expanded/simplified.

Copy link
Member

@MetaLang MetaLang left a comment

Choose a reason for hiding this comment

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

LGTM with room for further improvements.

Copy link
Member

@DmitryOlshansky DmitryOlshansky left a comment

Choose a reason for hiding this comment

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

LGTM

@JackStouffer
Copy link
Contributor Author

Auto-merge toggled on

@ibuclaw
Copy link
Member

ibuclaw commented Aug 3, 2017

This caused a regression.

https://issues.dlang.org/show_bug.cgi?id=17712

@ghost
Copy link

ghost commented Jun 11, 2018

This change make things working again.

@@ -4250,11 +4250,11 @@ private S textImpl(S, U...)(U args)
         {
             static if (
                 is(Unqual!(typeof(arg)) == uint) || is(Unqual!(typeof(arg)) == ulong) ||
                 is(Unqual!(typeof(arg)) == int) || is(Unqual!(typeof(arg)) == long)
             )
-                app.put(arg.toChars);
+                app.put(textImpl!(S)(arg));
             else
                 app.put(to!S(arg));
         }
 
         return app.data;

Maybe some templates got discarded because of variadic arg here ?

@ghost
Copy link

ghost commented Jun 11, 2018

That maybe more simply because toChars is not called anymore.

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

Labels

Review:Trivial typos, formatting, comments Severity:Optimization

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants