Menu

#551 CONCAT1 of ByteArrays

closed
5
2008-07-23
2008-04-28
No

So far, the CONCAT1 instruction goes through the string reps of all objects, even when all are byte arrays. This compromises performance in heavy byte-juggling code since it means converting back and forth to UTF-8. This is odd since [string range] et al. do have an optimization for the byte array case.

The attached patch (against 8.5.1) fixes that, by first looping through all to-be-concatenated objects to check whether they are all byte arrays (or empty strings).
In that case, all operations are done at byte array level, while keeping the very same logic regarding in-place operation if possible (unshared first object).

Moreover, the inclusion of the empty string case allows for the "K-free K" idiom $x[set x {}], which in the end allows the following in-place byte array growing operation:

set x $x[set x {}]$y

Test suite OK, and stepped through gdb to check most branches.

Discussion

  • Alexandre Ferrieux

    CONCAT1-byte-arrays patch

     
  • Alexandre Ferrieux

    • assigned_to: dkf --> msofer
     
  • Alexandre Ferrieux

    Logged In: YES
    user_id=496139
    Originator: YES

    Miguel, reassigning this to you on Donal's advice.
    Hope you don't mind :-}

     
  • Don Porter

    Don Porter - 2008-06-27

    Logged In: YES
    user_id=80530
    Originator: NO

    when applied to HEAD, the compiler has
    some gripes about this patch:

    /home/dgp/cvs/tcl/unix/../generic/tclExecute.c: In function ‘TclExecuteByteCode’:
    /home/dgp/cvs/tcl/unix/../generic/tclExecute.c:2221: warning: pointer targets in assignment differ in signedness
    /home/dgp/cvs/tcl/unix/../generic/tclExecute.c:2224: warning: pointer targets in assignment differ in signedness
    /home/dgp/cvs/tcl/unix/../generic/tclExecute.c:2230: warning: pointer targets in assignment differ in signedness
    /home/dgp/cvs/tcl/unix/../generic/tclExecute.c:2244: warning: pointer targets in assignment differ in signedness

     
  • miguel sofer

    miguel sofer - 2008-06-28

    Logged In: YES
    user_id=148712
    Originator: NO

    This looks OK for commit to HEAD after fixing those warnings; Alex, please do.

    It would be nice to insure that the testsuite covers any possible gotcha, eg different combinations of concat'ing bytearrays, empties and other things at first/other places.

    It'd be also nice to check if tclbench will expose the improvement, and to write a small bench to include there if not.

     
  • miguel sofer

    miguel sofer - 2008-06-28
    • assigned_to: msofer --> ferrieux
     
  • Alexandre Ferrieux

    fixed signed char*

     
  • Alexandre Ferrieux

    Logged In: YES
    user_id=496139
    Originator: YES

    Thanks for the green light.

    The just attached update fixes the warnings.
    FWIW, these warnings don't appear with msys/mingw, based on gcc 3.4.2.
    Maybe you compiled on unix with a more modern gcc ?

    Will now do my fist solo flight with non-anon cvs :-)
    File Added: binconcat2.patch

     
  • Alexandre Ferrieux

    Logged In: YES
    user_id=496139
    Originator: YES

    Committed to HEAD.
    Will now prepare test and tclbench cases.

     
  • Alexandre Ferrieux

    Logged In: YES
    user_id=496139
    Originator: YES

    Test suite updated with a relative speed test (detect overhead of conversions).

     
  • Alexandre Ferrieux

    • status: open --> closed