machinekit / machinekit-cnc

CNC stack split out into a separate package
Other
60 stars 37 forks source link

Interp::read_real_number is very inefficient #24

Open ArcEye opened 6 years ago

ArcEye commented 6 years ago

Issue by powool Sun Jun 14 13:25:16 2015 Originally opened as https://github.com/machinekit/machinekit/issues/685


src/emc/rs274ngc/interp_read.cc Interp::read_real_number() is very inefficient: it takes about 10X as long as it should.

C++ string stream libraries are very slow, and this method additionally causes needless new/delete as it is shuffling the string around.

I don't have a good build environment, otherwise I'd offer a pull request, but here's my standalone test file of two functions that I believe do the same thing. This needs to be verified with someone comfortable in running regression tests.

To test, compile the below code and run like this:

./float 1 3.1415926 ./float 2 3.1415926

The second float function uses strtod() with no additional memcpy, new, delete or anything, and shows as being about 9X as fast on my beaglebone black.

In benchmarking before and after gcode load times, the difference is much less - since I did this a month ago, I don't remember exactly how much - I think on the order of 10%.

There are many instances of strspn() being used - any that are called for every character of the input gcode also need to be re-written, especially ones such as at /src/emc/rs274ngc/interp_remap.cc near line 403, although from looking at the context, I doubt it gets called much in my use case (3D printing).

#include <iostream>
#include <stdlib.h>
#include <string.h>
#include <string>
#include <sstream>

int read_real_number1(const char *line, //!< string: line of RS274/NGC code being processed
    int *counter,       //!< pointer to a counter for position on the line
    double *double_ptr) //!< pointer to double to be read
{
    const char *start;
    size_t after;

    start = line + *counter;

    after = strspn(start, "+-");
    after = strspn(start+after, "0123456789.") + after;

    std::string st(start, start+after);
    std::stringstream s(st);
    double val;
    if(!(s >> val))
    {
        std::cout << "bad number format (conversion failed) parsing '" << line << "'\n";
        return 1;
    }
    if(s.get() != std::char_traits<char>::eof())
    {
        std::cout << "bad number format (conversion failed) parsing '" << line << "'\n";
        return 1;
    }

    *double_ptr = val;
    *counter = start + after - line;
    //fprintf(stderr, "got %f   rest of line=%s\n", val, line+*counter);
    return 0;
}

int read_real_number2(const char *line, //!< string: line of RS274/NGC code being processed
    int *counter,       //!< pointer to a counter for position on the line
    double *double_ptr) //!< pointer to double to be read
{
    const char *start;
    size_t after;

    double result = 0;
    char *endptr;
    result = strtod(line + *counter, &endptr);

    if(endptr == NULL)
    {
        std::cout << "bad number format (conversion failed) parsing '" << line << "'\n";
        return 1;
    }

    *counter += (endptr - (line + *counter));

    *double_ptr = result;

    return 0;
}

int main(int argc, const char **argv)
{
    if(argc != 3)
    {
        std::cout << "usage: " << argv[0] << " [1|2] <number>\n";
        exit(1);
    }

    double number = 0;
    int counter = 0;
    int result;
    int i;
    for(i = 0; i < 500000 ; i++)
    {
        number = 0;
        counter = 0;

        if(argv[1][0]=='1')
            result = read_real_number1(argv[2], &counter, &number);
        else
            result = read_real_number2(argv[2], &counter, &number);
    }

    std::cout << "result = " << result << ", counter = " << counter << ", number = " << number << "\n";;

    exit(0);
}
ArcEye commented 6 years ago

Comment by mhaberler Sun Jun 14 14:52:03 2015


I have noticed that too, and having that improved helps

if you do a PR, please assure you have runtests for all floating point formats to verify there is no change in semantics

you could do those with the rs274 standalone interpreter

ArcEye commented 6 years ago

Comment by mhaberler Thu Jul 9 05:25:53 2015


@powool - how is the progress on the issue?

ArcEye commented 6 years ago

Comment by powool Thu Jul 9 15:12:20 2015


No real progress, sorry - I basically parked the work I did so far in this issue, so I wouldn't lose it on my laptop.

What I did so far was attempt to set up a clean build environment on my ubuntu 14.04 setup, and got stopped there. I'll try again soon and then be able to test my changes.