www.digitalmars.com         C & C++   DMDScript  

digitalmars.D.bugs - [Issue 2008] New: Poor optimization of functions with ref parameters

reply d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=2008

           Summary: Poor optimization of functions with ref parameters
           Product: D
           Version: 1.028
          Platform: PC
        OS/Version: Windows
            Status: NEW
          Severity: normal
          Priority: P2
         Component: DMD
        AssignedTo: bugzilla digitalmars.com
        ReportedBy: aarti interia.pl


Functions with ref parameters can be up to 2x slower than functions which
parameters are not passed by reference. It is especially visible for programs
which are compiled with all optimizations on:
-O
-inline
-release

Test program:
------------------------

char peek_ref(ref char[] a) {
    return a[0];
}

char peek_noref(char[] a) {
    return a[0];
}


ulong rdtsc() {
    asm {
        naked;
        rdtsc;
        ret;
    }
}

void main() {
    char[] tst = "test string";
    char c;
    long starttime;

    writef("Direct: ");
    starttime = rdtsc;
    for(int i=0; i<1000; i++)
        c = tst[0];
    writefln(c, " - time: ", rdtsc - starttime);

    writef("With ref: ");
    starttime = rdtsc;
    for(int i=0; i<1000; i++)
        c = tst.peek_ref();
    writefln(c, " - time: ", rdtsc - starttime);

    writef("Without ref: ");
    starttime = rdtsc;
    for(int i=0; i<1000; i++)
        c = tst.peek_noref();
    writefln(c, " - time: ", rdtsc - starttime);
}

----------------------


-- 
Apr 18 2008
next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=2008





------- Comment #1 from wbaxter gmail.com  2008-04-18 18:22 -------
I believe the problem is that ref parameters disable inlining in DMD.  Or so I
think I heard somewhere before.


-- 
Apr 18 2008
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=2008


David Simcha <dsimcha yahoo.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |dsimcha yahoo.com




--- Comment #2 from David Simcha <dsimcha yahoo.com>  2009-05-15 21:06:18 PDT
---
This is true, and also applies to D2.  Apparently ref prevents inlining.  It's
somewhere in the comments in inline.c.  Why, I don't know.  The weird thing is
that apparently DMD inlines stuff with pointer parameters, and ref parameters
are just syntactic sugar for pointers.  Here's a test program and the
disassembly.

import std.stdio;

// Shouldn't this generate *exactly* the same code as ptrSwap?
void swap(T)(ref T a, ref T b) {
    T temp = a;
    a = b;
    b = temp;
}

void ptrSwap(T)(T* a, T* b) {
    T temp = *a;
    *a = *b;
    *b = temp;
}

void main() {
    uint a, b;
    swap(a, b);
    ptrSwap(&a, &b);
    writeln(a); // Keep DMD from optimizing out ptrSwap entirely.
}


Here's the disassembly of the relevant portion:

  COMDEF __Dmain
        push    eax                                     ; 
        push    eax                                     ; 
        xor     eax, eax                                ;
        push    ebx                                     ; 
        lea     ecx, [esp+4H]                           ; 
        mov     dword ptr [esp+4H], eax                 ; 
        mov     dword ptr [esp+8H], eax                 ; 
        push    ecx                                     ; 
        lea     eax, [esp+0CH]                          ; 
        call    _D5test711__T4swapTkZ4swapFKkKkZv       ; 
        mov     edx, dword ptr [esp+4H]                 ; 
        mov     ebx, dword ptr [esp+8H]                 ; 
        mov     dword ptr [esp+4H], ebx                 ; 
        mov     eax, offset FLAT:_main                  ; 
        mov     dword ptr [esp+8H], edx                 ; 
        push    ebx                                     ;
        push    10                                      ;
        call    _D3std5stdio4File14__T5writeTkTaZ5writeMFkaZv;
        xor     eax, eax                                ; 
        pop     ebx                                     ; 
        add     esp, 8                                  ;
        ret                                             ; 
__Dmain ENDP

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
May 15 2009
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=2008


Eldar Insafutdinov <e.insafutdinov gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |e.insafutdinov gmail.com


--- Comment #3 from Eldar Insafutdinov <e.insafutdinov gmail.com> 2010-01-24
09:39:04 PST ---
Recent change to dmd forced everybody to use opEquals with ref arguments. That
leaves no other option than the poorly optimised one. Isn't it time to fix this
bug?

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Jan 24 2010
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=2008



--- Comment #4 from David Simcha <dsimcha yahoo.com> 2010-01-24 16:00:12 PST ---
(In reply to comment #3)
 Recent change to dmd forced everybody to use opEquals with ref arguments. That
 leaves no other option than the poorly optimised one. Isn't it time to fix this
 bug?

I think the more important fix is to start allowing opEquals with non-ref arguments again. Only allowing ref arguments is ridiculous because it forces the argument to be an lvalue, thus creating a horribly ugly inconsistency with builtin types. -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
Jan 24 2010
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=2008



--- Comment #5 from Eldar Insafutdinov <e.insafutdinov gmail.com> 2010-01-24
16:06:02 PST ---
(In reply to comment #4)
 (In reply to comment #3)
 Recent change to dmd forced everybody to use opEquals with ref arguments. That
 leaves no other option than the poorly optimised one. Isn't it time to fix this
 bug?

I think the more important fix is to start allowing opEquals with non-ref arguments again. Only allowing ref arguments is ridiculous because it forces the argument to be an lvalue, thus creating a horribly ugly inconsistency with builtin types.

That's true. Perhaps functions with signature (const ref T) could accept rvalues as well, as the semantics is equivalent to (T)? -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
Jan 24 2010
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=2008


Brad Roberts <braddr puremagic.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
         AssignedTo|nobody puremagic.com        |braddr puremagic.com


--- Comment #6 from Brad Roberts <braddr puremagic.com> 2010-05-09 04:16:48 PDT
---
Created an attachment (id=626)
a potential patch (lots of debugging code left in for now) 

This diff likely needs to be used in conjunction with the patch in bug 4149.  I
haven't tested it separately to find out.

I also haven't tested it with more than the simple test case in bug 4149.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
May 09 2010
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=2008



--- Comment #7 from Brad Roberts <braddr puremagic.com> 2010-05-09 04:35:43 PDT
---
Testing with the code from Marcin, using -O -inline and fixing several bugs due
to changes since 2008:

Direct: t - time: 4319
With ref: t - time: 4319
Without ref: t - time: 4382

The code changes (not perfect, but enough to get it to compile):
  1) add .dup to the "test string" literal
  2) s/writefln/writeln/

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
May 09 2010
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=2008



--- Comment #8 from Brad Roberts <braddr puremagic.com> 2010-05-09 04:41:38 PDT
---
Testing with David's code, with -O -inline, here's the new _Dmain:

_Dmain:
                push    EBP
                mov     EBP,ESP
                mov     EAX,offset
FLAT:_D3std5stdio6stdoutS3std5stdio4File SYM32
                push    0
                push    0Ah
                call    near ptr
_D3std5stdio4File14__T5writeTkTaZ5writeMFkaZv PC32
                xor     EAX,EAX
                pop     EBP
                ret

Without -O:
_Dmain:
                push    EBP
                mov     EBP,ESP
                sub     ESP,024h
                push    EBX
                push    ESI
                xor     EAX,EAX
                mov     -024h[EBP],EAX
                mov     -020h[EBP],EAX
                lea     ECX,-024h[EBP]
                mov     -01Ch[EBP],ECX
                lea     EDX,-020h[EBP]
                mov     -018h[EBP],EDX
                mov     EBX,[ECX]
                mov     -014h[EBP],EBX
                mov     ESI,[EDX]
                mov     [ECX],ESI
                mov     [EDX],EBX
                lea     EAX,-024h[EBP]
                mov     -010h[EBP],EAX
                lea     ECX,-020h[EBP]
                mov     -0Ch[EBP],ECX
                mov     EDX,[EAX]
                mov     -8[EBP],EDX
                mov     EBX,[ECX]
                mov     [EAX],EBX
                mov     [ECX],EDX
                mov     ESI,-024h[EBP]
                mov     -4[EBP],ESI
                push    ESI
                push    0Ah
                mov     EAX,offset
FLAT:_D3std5stdio6stdoutS3std5stdio4File SYM32
                call    near ptr
_D3std5stdio4File14__T5writeTkTaZ5writeMFkaZv PC32
                xor     EAX,EAX
                pop     ESI
                pop     EBX
                leave
                ret

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
May 09 2010
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=2008


Brad Roberts <braddr puremagic.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #626 is|0                           |1
           obsolete|                            |


--- Comment #9 from Brad Roberts <braddr puremagic.com> 2010-05-10 00:07:11 PDT
---
Created an attachment (id=627)
Enable inlining of functions with ref parameters, v2

The previous patch didn't pass the dmd test suite (still need to understand
why).

This newer patch isolates the changes to the inlining code to generate AST
closer to what would be manually written for hand inlined code.  I also dropped
all of the debugging code and unrelated code cleanup.  This version DOES pass
the dmd test suite.

I've also confirmed that it is NOT dependent on the patch attached to bug 4149.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
May 10 2010
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=2008



--- Comment #10 from Brad Roberts <braddr puremagic.com> 2010-05-16 18:03:11
PDT ---
Created an attachment (id=633)
add support for out parameters as well (so both ref and out)

The previous patch only added support for ref parameters.  This updated patch
also includes adding support for inlining of functions that have out
parameters.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
May 16 2010
prev sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=2008


Walter Bright <bugzilla digitalmars.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
                 CC|                            |bugzilla digitalmars.com
         Resolution|                            |FIXED
           Severity|normal                      |enhancement


--- Comment #11 from Walter Bright <bugzilla digitalmars.com> 2010-05-21
18:34:35 PDT ---
changeset 497. Does static arrays too.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
May 21 2010