Merge pull request #299 from jayrm/optimize-StrConcatByref

Optimize Byref String Concatenation

Author: jayrm

changelog.txt

@@ -38,6 +38,7 @@ Version 1.08.0
 - fbc: internal changes to optimize away unused call results
 - github #203: allow casts of addresses on static initializers 
 - only write debug line information for statements and don't write comments / empty lines / directives for top level source code in assembly debug ouput
+- optimize byref 'm += s' string concatenations to fb_StrConcatByref() which will check for same string descriptor at run-time which can't be determined at compile time for byref parameters.
 
 [added]
 - extern "rtlib": respects the parent namespace, uses default fb calling convention and C style name mangling
@@ -73,6 +74,7 @@ Version 1.08.0
 - __FB_ARG_EXTRACT__( index, args... ) builtin macro (adeyblue)
 - __FB_X86__ intrinsic define on x86 and x86_64
 - added warning 'FOR counter variable is unable to exceed limit value' on constant end value for loops to help avoid infinite loops, e.g. for i as ubyte = 0 to 255
+- internal rtlib function fb_LEFTSELF( string, n ) to reduce the size of a string without reallocating the buffer
 
 [fixed]
 - makefile: under MSYS2 (and friends), TARGET_ARCH is now identified from shell's default target architecture instead of shell's host architecture
@@ -121,6 +123,7 @@ Version 1.08.0
 - gcc backend: fix GOSUB causing crash/exception on win64 - setjmp/longjmp failed on mingw-w64 64-bit and needed to be passed 2 parameters instead of 1
 - fix __FB_EVAL__() incorrectly reading past the end of the expression, and report errors in expressions
 - C backend: switch to .text section after writing the exports to the C file in the explicit asm block.  gcc can move sections around with optimizations and there is a change between 7.x and 8.x that causes issue with where the directive section is located
+- sf.net #917: optimize 'm += s' string concatenations to fix the long compile times in the gcc backend (which makes heavy use of string building).
 
 
 Version 1.07.0

src/compiler/ast-optimize.bas

@@ -1464,9 +1464,10 @@ private function hOptStrAssignment _
 		byval r as ASTNODE ptr _
 	) as ASTNODE ptr
 
-	dim as integer optimize = any, is_wstr = any
+	dim as integer optimize = any, is_byref = any, is_wstr = any
 
 	optimize = FALSE
+	is_byref = FALSE
 
 	'' is right side a bin operation?
 	if( r->class = AST_NODECLASS_BOP ) then
@@ -1486,14 +1487,35 @@ private function hOptStrAssignment _
 				end if
 			end if
 
+		case AST_NODECLASS_DEREF
+			'' check if we can optimize to fb_StrConcatByref()
+			'' looking specifically for a = a + b
+			'' where both a & b are STRINGS but we possibly
+			'' don't know until run time if a and b could 
+			'' be the same descriptor.
+			if( astIsTreeEqual( l, r->l ) ) then
+				sym = astGetSymbol( l->l )
+				if( sym <> NULL ) then
+					if( astGetDataType( l ) = FB_DATATYPE_STRING ) then
+						optimize = TRUE
+						is_byref = TRUE
+					end if
+				end if
+			end if
+
 		case AST_NODECLASS_FIELD, AST_NODECLASS_IIF
 			select case as const l->l->class
 			case AST_NODECLASS_VAR, AST_NODECLASS_IDX
 
 				if( astIsTreeEqual( l, r->l ) ) then
 					sym = astGetSymbol( l )
 					if( sym <> NULL ) then
-						optimize = astIsSymbolOnTree( sym, r->r ) = FALSE
+						if( astGetDataType( l ) = FB_DATATYPE_STRING ) then
+							optimize = TRUE
+							is_byref = TRUE
+						else
+							optimize = astIsSymbolOnTree( sym, r->r ) = FALSE
+						end if
 					end if
 				end if
 
@@ -1513,13 +1535,13 @@ private function hOptStrAssignment _
 		if( hIsMultStrConcat( l, r ) ) then
 			function = hOptStrMultConcat( l, l, r, is_wstr )
 		else
-			''	=            f() -- concatassign
+			''	=            f() -- concatassign | concatbyref
 			'' / \           / \
 			''a   +    =>   a   expr
 			''   / \
 			''  a   expr
 			if( is_wstr = FALSE ) then
-				function = rtlStrConcatAssign( l, astUpdStrConcat( r ) )
+				function = rtlStrConcatAssign( l, astUpdStrConcat( r ), is_byref )
 			else
 				function = rtlWstrConcatAssign( l, astUpdStrConcat( r ) )
 			end if

src/compiler/ir-hlc.bas

@@ -3772,7 +3772,14 @@ private sub _emitVarIniScopeEnd( )
 	'' Trim separator at the end, to make the output look a bit more clean
 	'' (this isn't needed though, since the extra comma is allowed in C)
 	if( right( ctx.varini, 2 ) = ", " ) then
-		ctx.varini = left( ctx.varini, len( ctx.varini ) - 2 )
+		'' fbc doesn't optimize this very well and on long strings it becomes expensive
+		'' it's built in to the run-time, so revert to normal expression
+		'' if we don't have fb_leftself defined yet.
+		#ifndef fb_leftself
+			ctx.varini = left( ctx.varini, len( ctx.varini ) - 2 )
+		#else
+			fb_leftself( ctx.varini, len( ctx.varini ) - 2 ) 
+		#endif		''     
 	end if
 
 	ctx.varini += " }"

src/compiler/rtl-string.bas

@@ -143,6 +143,23 @@
 				( typeSetIsConst( FB_DATATYPE_INTEGER ), FB_PARAMMODE_BYVAL, FALSE ) _
 			} _
  		), _
+		/' function fb_StrConcatByref(
+				byref str1 as const any, byval str1_size as const integer, _
+				byref str2 as const any, byval str2_size as const integer, _
+				byval fillrem as const long = 1 ) as string '/ _
+		( _
+			@FB_RTL_STRCONCATBYREF, NULL, _
+			FB_DATATYPE_STRING, FB_FUNCMODE_FBCALL, _
+			NULL, FB_RTL_OPT_NONE, _
+			5, _
+			{ _
+				( FB_DATATYPE_VOID, FB_PARAMMODE_BYREF, FALSE ), _
+				( typeSetIsConst( FB_DATATYPE_INTEGER ), FB_PARAMMODE_BYVAL, FALSE ), _
+				( typeSetIsConst( FB_DATATYPE_VOID ), FB_PARAMMODE_BYREF, FALSE ), _
+				( typeSetIsConst( FB_DATATYPE_INTEGER ), FB_PARAMMODE_BYVAL, FALSE ), _
+				( typeSetIsConst( FB_DATATYPE_LONG ), FB_PARAMMODE_BYVAL, TRUE, 1 ) _
+			} _
+ 		), _
 		/' function fb_WstrConcat( byval str1 as const wstring ptr, byval str2 as const wstring ptr ) as wstring '/ _
 		( _
 			@FB_RTL_WSTRCONCAT, NULL, _
@@ -1840,7 +1857,7 @@
 			{ _
 				( typeSetIsConst( FB_DATATYPE_LONG ), FB_PARAMMODE_BYVAL, FALSE ) _
 			} _
- 		), _
+		), _
 		/' function fb_MKLONGINT( byval number as const longint ) as string '/ _
 		( _
 			@FB_RTL_MKLONGINT, NULL, _
@@ -1850,7 +1867,7 @@
 			{ _
 				( typeSetIsConst( FB_DATATYPE_LONGINT ), FB_PARAMMODE_BYVAL, FALSE ) _
 			} _
- 		), _
+		), _
 		/' function left overload( byref str as const string, byval chars as const integer ) as string '/ _
 		( _
 			@"left", @"fb_LEFT", _
@@ -1861,7 +1878,7 @@
 				( typeSetIsConst( FB_DATATYPE_STRING ), FB_PARAMMODE_BYREF, FALSE ), _
 				( typeSetIsConst( FB_DATATYPE_INTEGER ), FB_PARAMMODE_BYVAL, FALSE ) _
 			} _
- 		), _
+		), _
 		/' function left overload( byref str as const wstring, byval chars as const integer ) as wstring '/ _
 		( _
 			@"left", @"fb_WstrLeft", _
@@ -1873,6 +1890,17 @@
 				( typeSetIsConst( FB_DATATYPE_INTEGER ), FB_PARAMMODE_BYVAL, FALSE ) _
 			} _
  		), _
+		/' sub fb_leftself overload( byref str as string, byval chars as const integer )'/ _
+		( _
+			@"fb_LeftSelf", @"fb_LEFTSELF", _
+			FB_DATATYPE_VOID, FB_FUNCMODE_FBCALL, _
+			NULL, FB_RTL_OPT_OVER or FB_RTL_OPT_NOQB, _
+			2, _
+			{ _
+				( typeSetIsConst( FB_DATATYPE_STRING ), FB_PARAMMODE_BYREF, FALSE ), _
+				( typeSetIsConst( FB_DATATYPE_INTEGER ), FB_PARAMMODE_BYVAL, FALSE ) _
+			} _
+ 		), _
 		/' function right overload( byref str as const string, byval chars as const integer ) as string '/ _
 		( _
 			@"right", @"fb_RIGHT", _
@@ -1914,7 +1942,7 @@
 			{ _
 				( typeSetIsConst( FB_DATATYPE_INTEGER ), FB_PARAMMODE_BYVAL, FALSE ) _
 			} _
- 		), _
+		), _
 		/' function fb_StrLcase2( byref src as const string, byval mode as const long = 0 ) as string '/ _
 		( _
 			@FB_RTL_STRLCASE2, NULL, _
@@ -1925,7 +1953,7 @@
 				( typeSetIsConst( FB_DATATYPE_STRING ), FB_PARAMMODE_BYREF, FALSE ), _
 				( typeSetIsConst( FB_DATATYPE_LONG ), FB_PARAMMODE_BYVAL, TRUE, 0 ) _
 			} _
- 		), _
+		), _
 		/' function fb_WstrLcase2( byref str as const wstring, byval mode as const long = 0 ) as wstring '/ _
 		( _
 			@FB_RTL_WSTRLCASE2, NULL, _
@@ -1936,7 +1964,7 @@
 				( typeSetIsConst( FB_DATATYPE_WCHAR ), FB_PARAMMODE_BYREF, FALSE ), _
 				( typeSetIsConst( FB_DATATYPE_LONG ), FB_PARAMMODE_BYVAL, TRUE, 0 ) _
 			} _
- 		), _
+		), _
 		/' function fb_StrUcase2( byref src as const string, byval mode as const long = 0 ) as string '/ _
 		( _
 			@FB_RTL_STRUCASE2, NULL, _
@@ -2316,7 +2344,8 @@ end function
 function rtlStrConcatAssign _
 	( _
 		byval dst as ASTNODE ptr, _
-		byval src as ASTNODE ptr _
+		byval src as ASTNODE ptr, _
+		byval isConcatByref as integer = FALSE _
 	) as ASTNODE ptr
 
     dim as ASTNODE ptr proc = any
@@ -2325,7 +2354,11 @@ function rtlStrConcatAssign _
 
 	function = NULL
 
-	proc = astNewCALL( PROCLOOKUP( STRCONCATASSIGN ) )
+	if( isConcatByref ) then
+		proc = astNewCALL( PROCLOOKUP( STRCONCATBYREF ) )
+	else
+		proc = astNewCALL( PROCLOOKUP( STRCONCATASSIGN ) )
+	end if
 
    	ddtype = astGetDataType( dst )
 

src/compiler/rtl.bi

@@ -9,6 +9,7 @@
 #define FB_RTL_HSTRDELTEMP				"fb_hStrDelTemp"
 #define FB_RTL_STRASSIGN				"fb_StrAssign"
 #define FB_RTL_STRCONCAT 				"fb_StrConcat"
+#define FB_RTL_STRCONCATBYREF			"fb_StrConcatByref"
 #define FB_RTL_STRCOMPARE				"fb_StrCompare"
 #define FB_RTL_STRCONCATASSIGN			"fb_StrConcatAssign"
 #define FB_RTL_STRALLOCTMPRES			"fb_StrAllocTempResult"
@@ -419,6 +420,7 @@ enum FB_RTL_IDX
 	FB_RTL_IDX_HSTRDELTEMP
 	FB_RTL_IDX_STRASSIGN
 	FB_RTL_IDX_STRCONCAT
+	FB_RTL_IDX_STRCONCATBYREF
 	FB_RTL_IDX_STRCOMPARE
 	FB_RTL_IDX_STRCONCATASSIGN
 	FB_RTL_IDX_STRALLOCTMPRES
@@ -960,7 +962,8 @@ declare function rtlWstrAssign _
 declare function rtlStrConcatAssign _
 	( _
 		byval dst as ASTNODE ptr, _
-		byval src as ASTNODE ptr _
+		byval src as ASTNODE ptr, _
+		byval isConcatByref as integer = FALSE _
 	) as ASTNODE ptr
 
 declare function rtlWstrConcatAssign _

src/rtlib/fb_string.h

@@ -123,6 +123,7 @@ FBCALL void        *fb_StrAssignEx      ( void *dst, ssize_t dst_size, void *src
 FBCALL void         fb_StrDelete        ( FBSTRING *str );
 FBCALL FBSTRING    *fb_StrConcat        ( FBSTRING *dst, void *str1, ssize_t str1_size, void *str2, ssize_t str2_size );
 FBCALL void        *fb_StrConcatAssign  ( void *dst, ssize_t dst_size, void *src, ssize_t src_size, int fillrem );
+FBCALL void        *fb_StrConcatByref   ( void *dst, ssize_t dst_size, void *src, ssize_t src_size, int fillrem );
 FBCALL int          fb_StrCompare       ( void *str1, ssize_t str1_size, void *str2, ssize_t str2_size );
 FBCALL FBSTRING    *fb_StrAllocTempResult ( FBSTRING *src );
 FBCALL FBSTRING    *fb_StrAllocTempDescF( char *str, ssize_t str_size );
@@ -244,6 +245,7 @@ FBCALL FBSTRING    *fb_MKI              ( ssize_t num );
 FBCALL FBSTRING    *fb_MKL              ( int num );
 FBCALL FBSTRING    *fb_MKLONGINT        ( long long num );
 FBCALL FBSTRING    *fb_LEFT             ( FBSTRING *str, ssize_t chars );
+FBCALL void         fb_LEFTSELF         ( FBSTRING *str, ssize_t chars );
 FBCALL FBSTRING    *fb_RIGHT            ( FBSTRING *str, ssize_t chars );
 FBCALL FBSTRING    *fb_SPACE            ( ssize_t chars );
 FBCALL FBSTRING    *fb_LTRIM            ( FBSTRING *str );

src/rtlib/str_concatbyref.c

@@ -0,0 +1,83 @@
+/* 
+	run-time optimization for:
+
+		dim shared m as string
+		sub proc( byref s as string, byref t as string )
+			'' these are optimized to fb_StrConcatByref() 
+			m += s
+			s += t
+		end sub
+
+		dim as string a, b
+		proc( m, a )
+		proc( a, a )
+		proc( a, b )
+
+	it can't be determined at compile time within proc() if the strings passed
+	to proc() are the same strings.  Might be able to use fb_StrConcatAssign() 
+	but only if it is known that the strings are different.  Otherwise the
+	contents of the string could be destroyed before the are copied.  If the
+	strings are the same, then extend the buffer and copy the first part of the
+	string to the second half.
+
+	We should only get here if the left hand side variable was an FBSTRING type
+	and we should expect a string descriptor.
+*/
+
+#include "fb.h"
+
+FBCALL void *fb_StrConcatByref
+	(
+		void *dst,
+		ssize_t dst_size,
+		void *src,
+		ssize_t src_size,
+		int fillrem
+	)
+{
+	char *dst_ptr;
+	const char *src_ptr;
+	ssize_t dst_len, src_len;
+
+	/* dst should always be var-len string */
+	DBG_ASSERT( dst_size == -1 );
+
+	/* dst */
+	FB_STRSETUP_FIX( dst, dst_size, dst_ptr, dst_len );
+
+	/* src */
+	FB_STRSETUP_FIX( src, src_size, src_ptr, src_len );
+
+	/* Are dst & src same same data? */
+	if( dst == src || dst_ptr == src_ptr )
+	{
+		FBSTRING *str = dst;
+
+		FB_STRLOCK();
+		
+		if( fb_hStrRealloc( str, dst_len + src_len, FB_TRUE ) )
+		{
+			/* recalculate dst */
+			FB_STRSETUP_FIX( str, dst_size, dst_ptr, dst_len );
+
+			/* copy start of dst to second half */
+			FB_MEMCPYX( dst_ptr + dst_len, dst, dst_len );
+
+			fb_hStrSetLength( str, dst_len + dst_len );
+
+			str->data[dst_len + dst_len] = '\0';
+		}
+
+		/* delete temps? */
+		if( dst_size == -1 )
+			fb_hStrDelTemp_NoLock( (FBSTRING *)dst );
+		if( src_size == -1 )
+			fb_hStrDelTemp_NoLock( (FBSTRING *)src );
+
+		FB_STRUNLOCK();
+
+		return dst;
+	}
+
+	return fb_StrConcatAssign( dst, dst_size, src, src_size, fillrem );
+}