3
\$\begingroup\$

This opens a file and writes 5120 times a string to it, and if the file doesn't exist it creates a new file. The name of the file and the string are chosen randomly. I would like to know if there is a way to speed up this code. It takes about 10 seconds at the moment. I was wondering if I did a recursion in the escreve() function or if I create some space in memory with malloc() on the main and free it before each cycle. If it will speed the thing up.

#define linhas 1024
#define vezes 5120
char *ficheiros[5]={"SO2014-0.txt","SO2014-1.txt","SO2014-2.txt","SO2014-3.txt","SO2014-4.txt"};
char *cadeias[10]={"aaaaaaaaa\n","bbbbbbbbb\n","ccccccccc\n","ddddddddd\n","eeeeeeeee\n","fffffffff\n","ggggggggg\n","hhhhhhhhh\n","iiiiiiiii\n","jjjjjjjjj\n"};

int random(int range)
{
    int num;
    num = rand() % range;
    return num;
}

void escreve(char *fi,char *cad){
    int l;
    int file = open(fi, O_WRONLY | O_CREAT, S_IRWXU | S_IROTH ) ;
    for(l=0;l<linhas;l++)
        write(file,cad, strlen(cad));
    close(file);
}


int main(){
    int x,y,l,i;
    srand(time(NULL));
for(i=0; i<vezes; i++){
    x= random(5);
        y=random(10);
        escreve(ficheiros[x],cadeias[y]);

}
return 0;
}
\$\endgroup\$
  • \$\begingroup\$ ok so after trying for a bit i figured that if i dont close(file) it speeds up alot. it takes 2 seconds now. but i think faster it's still possible. i also tryed the escreve function has a recurssion it didnt change the time. any ideas? \$\endgroup\$ – pedro condeço Oct 5 '14 at 23:56
  • \$\begingroup\$ Use English variable names,function names,comments. example : ficheiros -> file_names \$\endgroup\$ – bhathiya-perera Oct 6 '14 at 2:54
4
\$\begingroup\$

This is a very instructive example of wrong judgement. I understand (correct me if I am wrong) that you went for a low-level write to avoid seemingly unnecessary overhead of FILE * functions offered by a stdio library. Not only it made your code less portable, it in fact made id slower!

The net effect is that your code invokes a system call for each 10 characters.

System call is an extremely expensive operation. It involves switching memory maps from a user to a kernel space, and on top of that it faces who knows how many cache misses. This is exactly the problem stdio solves by internal buffering.

This is how your original code behaves:

~/projects/play/cr/pedro $ time ./original 

real    0m7.929s
user    0m0.484s
sys     0m7.432s

I changed opening a file to

FILE * file = fopen(fi, "w");

and writing to it to

    fputs(cad, file);

and the new timigs are

~/projects/play/cr/pedro $ time ./with-stdio 

real    0m0.908s
user    0m0.276s
sys     0m0.584s

PS To make it a code review:

You shall #include at least <sys/types.h>, <sys/stat.h>, <fcntl.h> and <time.h>.

| improve this answer | |
\$\endgroup\$
1
\$\begingroup\$

I haven't changed the performance aspect much, but I have nonetheless made lots of improvements with regard to syntax, readability, portability and conformity to modern C coding standards:

#include <errno.h>
#include <fcntl.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/stat.h>
#include <time.h>
#include <unistd.h>

// Compile with -std=c11 (or -std=c99)

// Use uppercase for constants
#define LINHAS 1024
#define VEZES 5120
// If you're going to hardcode your filenames, better make them macros too.
#define FICHEIROS {"SO2014-0.txt", \
                   "SO2014-1.txt", \
                   "SO2014-2.txt", \
                   "SO2014-3.txt", \
                   "SO2014-4.txt"}
#define CADEIAS {"aaaaaaaaa\n", \
                 "bbbbbbbbb\n", \
                 "ccccccccc\n", \
                 "ddddddddd\n", \
                 "eeeeeeeee\n", \
                 "fffffffff\n", \
                 "ggggggggg\n", \
                 "hhhhhhhhh\n", \
                 "iiiiiiiii\n", \
                 "jjjjjjjjj\n"}
#define CAD_SIZE 10

// Convenient macro for whenever you need to loop through a static size array.
// Calculated at compile time.
#define ELEMENT_C(a) (sizeof(a) / sizeof((a)[0]))

// Always include prototypes of all your functions.
// Try to use English for variables and function names as much as possible.
// I'm not sure what they mean, so I haven't translated them here though.
int main(void);
int random(const int range);
void escreve(const int fd, const char *cad);

// Functions without arguments should have the word "void" between parentheses.
int main(void) {
    // time_t is not guaranteed to be unsigned (I think), so cast explicitly
    // for portability.
    srand((unsigned) time(NULL));
    // No need to specify an array size. The compiler will count it by itself.
    const char *ficheiros[] = FICHEIROS,
               *cadeias[] = CADEIAS;

    // Open all files before writing anything. Your code calls open() for every
    // call to escreve(). Seems like a bad idea to me.
    int fds[ELEMENT_C(ficheiros)];
    // Being able to declare variables inside loops is a nice feature of C99
    // onward. In general, tight scoping can prevent many kinds of bugs and
    // makes debugging easier.
    for (unsigned i = 0; i < ELEMENT_C(ficheiros); i++) {
        fds[i] = open(ficheiros[i], O_WRONLY | O_CREAT, S_IRWXU | S_IROTH);
        // Always check the return value of library functions for errors.
        if (fds[i] == -1) {
            // Use strerror() or perror() to get useful debug information
            fprintf(stderr, "Failed to open() %s: %s\n", ficheiros[i],
                    strerror(errno));
            // Use these stdlib macros for more readable return values.
            return EXIT_FAILURE;
        }
    }

    for (int i = 0; i < VEZES; i++) {
        escreve(fds[random(ELEMENT_C(ficheiros))],
                cadeias[random(ELEMENT_C(cadeias))]);
    }

    // Close all files after writing everything.
    for (unsigned i = 0; i < ELEMENT_C(ficheiros); i++) {
        if (close(fds[i]) == -1) {
            fprintf(stderr, "Failed to close() %s: %s\n", ficheiros[i],
                    strerror(errno));
            return EXIT_FAILURE;
        }
    }
    // Don't forget to return your exit code at the end of main.
    return EXIT_SUCCESS;
}

int random(const int range) {
    // This is the proper way to use rand().
    // Your version suffers from rounding errors.
    return rand() / (double) (RAND_MAX + 1) * range;
}

void escreve(const int fd, const char *cad) {
    for (int i = 0; i < LINHAS; i++) {
        if (write(fd, cad, CAD_SIZE) == -1) {
            fprintf(stderr, "Failed to write() %s to file descriptor %d: %s\n",
                    cad, fd, strerror(errno));
            exit(EXIT_FAILURE);
        }
    }
}
| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.