Help with file manipulation.

I need to read in lines from a simple text file 100,000 at a time in a file that contains millions of lines.

I tried opening the file, reading 100,000 lines, manipulating the info, saving the manipulated info out to a new file, then reading the next 100,000 lines. The program crashed.

Is there some reason why the data file wouldn't remain open? I open it in Main, then call a subroutine to load the data (and others to manipulate and then save the data).

To get around it I have changed the program to open the file, load 100,000 lines, then load the remaining lines and simultaneously write them out to a new file. When I need the next 100,000 lines, I open the new file and repeat the process. The file shrinks with each load, but it takes FOREVER to do things this way. I calculate that the current iteration will take about 10 hours to run, compared to 2 hours for the previous iteration. And I only expect it to continue to get worse for several more iterations before it should get better.

The program spends the vast majority of it's time simply trying to access the data. The actual manipulation only takes a fraction of the time currently.

I need to get it to work with only opening the file once and just reading each 100,000 entries out sequentially.

Any ideas what may be wrong? Would it be better for me just to post a stripped version of the main and load functions?

Show your code that is having problems. Also possibly a small sample of your input file.

Jim

Quote:

---

Originally Posted by jimblumberg

Show your code that is having problems. Also possibly a small sample of your input file.

Jim

---

Emphasis on small.

I was going to try to mask the code, but then I thought, why? It's not as if this is some top secret project. It's just a personal project and if someone really wants to steal my idea, not only would I be extremely surprised, but I don't see why I should care either. Perhaps I should be embarrassed about the code being inelegant, but whatever. As I said, this is just a personal project, it doesn't have to be super efficient or fancy. But the inefficiency of the current load method DOES need to get fixed.

The program is attempting to create EVERY possible legal board for a game of Connect 4. Each iteration represents a specific turn in the game. I load all of the boards from the previous turn, remove any "winners" from the previous turn, then calculate every possible response for the current turn. It doesn't matter whether it would be "stupid" to make a certain play, only whether it is "legal" by the rules of the game.

The input files are all simply lines of text similar to these:

AeeeeeeRReeeeeeeeeeeeeeeeeeeeeeeeeeeeBBeeee
AeeeeeeReeeeeBBReeeeeeeeeeeeeeeeeeeeeeeeeee
AeeeeeeReeeeeBBeeeeReeeeeeeeeeeeeeeeeeeeeee
AeeeeeeReeeeeBBeeeeeeeeeeReeeeeeeeeeeeeeeee

These were pulled straight from a section of the output file for the 4th iteration. Each iteration fills in one more of the 'e'mpty values with either a 'B'lack or 'R'ed, according to the rules of Connect 4.

The program is a simple console app. Here's both the main() and LoadData() functions from the current version. When I tried to move the file manipulation into Main and just pass the file pointer into the load function, I couldn't get it to work correctly for some reason.

Code:

---

int main(int argc, char* argv[])
{
    bool    bMore          = false;
    int    x,
            col,
            row,
            iCount,
            iOffset;
    char    cField[80],
            cPlayer;
    BOARD  *sCurrBoard    = 0;

    strcpy (sBoard.cBoard, "Aeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee");

    for (x = LEVEL; x < LEVEL + DEPTH; x++)
    {
        iOffset = -OFFSET;
        if (x % 2 == 0)
            cPlayer = 'R';
        else
            cPlayer = 'B';

        cout << "Loading Data from level " << (x - 1) << ".\n";

        do
        {
            iOffset += OFFSET;
            cout << x << " = " << iOffset << " - " << iOffset + OFFSET << '\n';
            bMore = LoadData(x - 1, iOffset / OFFSET);

            if (iTotalBoards > 0)
            {
                sCurrBoard = sPrevBoard.next;
                iCount = 0;
                iNewTotal = 0;

                cout << "Calculating moves for level " << x << ".\n";
                while (sCurrBoard)
                {
                    if ( (iCount % 1000) == 0)
                        cout << "Board #" << iCount + iOffset << " of #"
                            << iTotalBoards + iOffset << ".\n";
                    strcpy (cField, sCurrBoard->cBoard);

                    for (col = 1; col < 8; col++)
                    {
                        row = 1;

                        while (    (row < 7)
                                && (cField[(col - 1) * 6 + row] != 'e')
                              )
                        {
                            row ++;
                        }

                        if (row < 7)
                        {
                            cField[(col - 1) * 6 + row] = cPlayer;
                            InsertBoard (cField, (BOARD*) calloc (1, sizeof (BOARD) ) );
                            iNewTotal++;
                            cField[(col - 1) * 6 + row] = 'e';
                        }
                    }
                    sCurrBoard = sCurrBoard->next;
                    iCount++;
                }

                cout << "Removing duplicates and sorting.\n";
                SortBoards ();
                cout << "Marking winners for level " << x << ".\n";
                MarkWinners ();
                cout << "Saving data for level " << x << ".\n";
                SaveData (x, iOffset / OFFSET);
                ClearData (sBoard.next);
                ClearData (sPrevBoard.next);
                sBoard.next = 0;
                sPrevBoard.next = 0;
            }
        } while (bMore);

        FileMerge (x, iOffset / OFFSET + 1);
    }

    return 0;
}

bool LoadData (int iLevel, int iOffset)
{
    char sData[360] = {0},
        sFile[80];
    BOARD *pNewBoard = 0,
          *pCurrBoard = 0;
    bool bMore = false;

    if (iOffset == 0)
        sprintf (sFile, "c:\\dev\\connect4\\connect4\\level%d.txt", iLevel);
    else
        sprintf (sFile, "c:\\dev\\connect4\\connect4\\level%dload%d.txt", iLevel, iOffset);

    ifstream Data(sFile);

    Data >> sData;
    pCurrBoard = &sPrevBoard;
    iTotalBoards = 0;

    while (    (strlen (sData) > 20)
            && (iTotalBoards < OFFSET)
          )
    {
        if (sData[0] == 'A')
        {
            pNewBoard = (BOARD*) calloc (1, sizeof (BOARD) );
            strcpy (pNewBoard->cBoard, sData);
            pCurrBoard->next = pNewBoard;
            pCurrBoard = pNewBoard;
            iTotalBoards++;
        }

        if (iTotalBoards < OFFSET)
            Data >> sData;
    }

    if (strlen (sData) > 20)
    {
        Data >> sData;

        if (strlen (sData) > 20)
        {
            sprintf (sFile, "c:\\dev\\connect4\\connect4\\level%dload%d.txt", iLevel, iOffset + 1);
            ofstream Data2 (sFile);

            while (strlen (sData) > 20)
            {
                Data2 << sData << '\n';
                Data >> sData;
            }
            bMore = true;
            Data2.close();
        }
    }

    Data.close();
    if (iOffset != 0)
    {
        sprintf (sFile, "c:\\dev\\connect4\\connect4\\level%dload%d.txt", iLevel, iOffset);
        remove (sFile);
    }

    return bMore;
}

---

As I think about it more, I suspect my problem was probably one of scope. Specifically that I declared the file pointer at a level where it was ending up out of scope before I got back to it. But I don't have the code from those attempts any more, so I can't be sure.

You need to show a complete program. What is a BOARD? Where is sBoard defined? What is sBoard? I see a lot of variables that do not seem to be defined. What header files are you including?

Jim

Accessories.h (entire contents):

Code:

---

typedef struct board
{
    char cBoard[50];
    struct board *next;

}BOARD;

typedef BOARD * pBoard;

typedef struct boardarray
{
    pBoard list;
    struct boardarray *next;
}BOARDARRAY;

typedef BOARDARRAY * pBoardArray;

bool LoadData (int iLevel, int iOffset);
void LoadData (int iLevel);
void SaveData (int iLevel, int iOffset);
void SaveData (int iLevel);
void FileMerge (int iLevel, int iOffset);
void InsertBoard (char cField[50], BOARD *sNewBoard);
void ClearData (BOARD *sFields);
void MarkWinners ();
void SortBoards ();
void SortMarked ();
void Debug (int iLevel, int iOffset);
void SortCheck (BOARD sBase);

---

Connect4.cpp (prior to main() ):

Code:

---

// Connect4.cpp : Defines the entry point for the console application.
//

#include "stdafx.h"
#include <string.h>
#include <fstream>
#include <iostream>
#include "Accessories.h"

using namespace std;

BOARD          sBoard,
                sPrevBoard;
int            iTotalBoards,
                iNewTotal;
BOARDARRAY      sBoardArray    = {0},
                *pCurrArray    = 0;

#define LEVEL 13
#define DEPTH 1
#define OFFSET 100000

---

Do you at some point free up all the memory you are dynamically allocating (calloc)?

If this is C++ you should be considering new/delete rather than calloc.

Is their a strong reason you are avoiding using std::string versus the C-style char arrays?

Why all the global variables?

Please explain the following:

Code:

---

                            InsertBoard (cField, (BOARD*) calloc (1, sizeof (BOARD) ) );

---

What are you actually trying to do, besides creating a memory leak, with this function call? Is this a C or C++ program. If it is a C++ program you should probably be using "new" instead of the C memory allocation functions.

You should also have include guards in your header file.

You should not be hiding the use of a pointer in a typedef.

Code:

---

typedef BOARD * pBoard; ////// Don't do this.

---

This does nothing but obscure your logic. If you want to use pointers, use pointers. The use of pointers should never be hidden, make them stand out like a sore thumb.

I also recommend that you start using std::strings instead of the C-strings, use vectors instead of the "arrays", and stop using global variables. Pass the correct variables to and from your functions as parameters.

Jim

You shouldn't use new. You should use std::string and std::vector. If you have to use new, use some smart pointer to manage the result. Do not use delete.

The only C++ things I see in any of those files is ifstream and cout. He uses structs, pointers, callo-... Who does that? Anyway. Um... This is C... sort of.

Quote:

---

Originally Posted by hk_mp5kpdw

Do you at some point free up all the memory you are dynamically allocating (calloc)?

If this is C++ you should be considering new/delete rather than calloc.

Is their a strong reason you are avoiding using std::string versus the C-style char arrays?

---

I'm not using a memory checker, so I won't swear to it, but I am cleaning up all of the memory as far as I know.

I use calloc and the rest due to familiarity. I learned C++ in school, but that was years ago. For the last 6 years I've been working at a company on a program written as a 16 bit application entirely in C, no C++. All my familiarity now is with what I use there. So my personal programs tend to reflect the same thing. It's just what I'm most familiar with.

I want to thank everyone for taking the time to respond. Thank you.

Quote:

---

Originally Posted by jimblumberg

Why all the global variables?

---

For variables I want to use throughout the program, I prefer globals over passing them to every function and sub-function I need to call.

Quote:

---

Originally Posted by jimblumberg

Please explain the following:

Code:

---

                            InsertBoard (cField, (BOARD*) calloc (1, sizeof (BOARD) ) );

---

What are you actually trying to do, besides creating a memory leak, with this function call? Is this a C or C++ program. If it is a C++ program you should probably be using "new" instead of the C memory allocation functions.

---

It's not a leak. At least, I'm fairly certain it's not. I'm allocating the memory and passing the pointer into the function. The value of the pointer then gets inserted into the growing list of data. At the end of the current run, the lists get processed and the memory freed then.

Quote:

---

Originally Posted by jimblumberg

You should also have include guards in your header file.

---

I don't understand what you mean here. What are "include guards"?

Quote:

---

Originally Posted by jimblumberg

You should not be hiding the use of a pointer in a typedef.

Code:

---

typedef BOARD * pBoard; ////// Don't do this.

---

This does nothing but obscure your logic. If you want to use pointers, use pointers. The use of pointers should never be hidden, make them stand out like a sore thumb.

---

To me, it does. But that's because of my background for the past 6 years. Anything starting with a 'p' is a pointer. To me, it jumps out immediately that I'm dealing with a pointer as soon as I see the 'p' at the start of pBoard.

I can fully understand why that's pretty specific to me and others wouldn't have the same reaction, but then again, this program wasn't ever intended for public perusal either. I was just writing it for my own use. The only reason it has become public is because I need help. I'm sorry, I really am, I understand it makes my code a bit harder for others to read, but it is what it is.

Quote:

---

Originally Posted by jimblumberg

I also recommend that you start using std::strings instead of the C-strings, use vectors instead of the "arrays"

---

If I was familiar with them, I might. I'm an excellent copycat. I can jump into an already written program, familiarize myself with it, and start adding to it, no problem. But if I don't have a framework to work from, I'm HORRIBLE at creating it from scratch. I also don't tend to go out and read reference works to learn new ways of doing something unless I don't know any way of doing it. If I already know a method that works, I use that method. I'm familiar with C-strings, so I use them.

Quote:

---

Originally Posted by jimblumberg

and stop using global variables. Pass the correct variables to and from your functions as parameters.

---

Why? What do I gain in this small program from passing multiple variables to every function over just making them global and only passing the few variables of lesser scope?

I'm asking honestly. What's the big deal?

The board counts are global for convenience. They're used extensively and I just didn't want to have to keep passing them into every function. But the board lists really should be global, in my opinion. They are the focus of the program. There's no point in the program where they "shouldn't" be visible or accessible. Without them, the program has no purpose. If they aren't "global", what is?

Quote:

---

It's not a leak. At least, I'm fairly certain it's not. I'm allocating the memory and passing the pointer into the function. The value of the pointer then gets inserted into the growing list of data. At the end of the current run, the lists get processed and the memory freed then.

---

Then why are you allocating the memory here in the function call, declare the pointer and allocate the memory for it in the function. What happens if the allocation fails?

Quote:

---

Why? What do I gain in this small program from passing multiple variables to every function over just making them global and only passing the few variables of lesser scope?

---

The problem with global variables is that they can be changed anywhere, in any function. Why must "every" function have access to these variables? By passing the variables into your functions with parameters you only allow certain functions to modify the variables making it easier to locate where the changes are occurring. The more local the variable the better off you will be. Using global variables even on small program reinforces bad practices. Once the program progress past just a "few" functions it will get harder and harder to troubleshoot, maintain, and extend your program.

Quote:

---

I don't understand what you mean here. What are "include guards"?

---

Include guards prevent the header file from being included more than once in a compilation unit. See this link: Include Guards

Jim

Okay, I tried again to convert the program to use the file once and leave it open until all the data is read. It crashed again.

Here's the modified main() and LoadData() functions.

Code:

---

int main(int argc, char* argv[])
{
    bool    bMore          = false;
    int    x,
            col,
            row,
            iCount,
            iOffset;
    char    cField[80],
            cPlayer,
            sFile[80];
    BOARD  *sCurrBoard    = 0;
    ifstream MainData;

    strcpy (sBoard.cBoard, "Aeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee");

    for (x = LEVEL; x < LEVEL + DEPTH; x++)
    {
        iOffset = -OFFSET;
        if (x % 2 == 0)
            cPlayer = 'R';
        else
            cPlayer = 'B';

        cout << "Loading Data from level " << (x - 1) << ".\n";
        sprintf (sFile, "c:\\dev\\connect4\\connect4\\level%d.txt", x - 1);
        MainData.open (sFile);

        do
        {
            iOffset += OFFSET;
            cout << x << " = " << iOffset << " - " << iOffset + OFFSET << '\n';
            bMore = LoadData(MainData);

            if (iTotalBoards > 0)
            {
                /* Same as before. Removed to save space. */
            }
        } while (bMore);

        MainData.close();
        FileMerge (x, iOffset / OFFSET + 1);
    }

    return 0;
}

---

Code:

---

bool LoadData (ifstream Data)
{
    char sData[360] = {0};
    BOARD *pNewBoard = 0,
          *pCurrBoard = 0;
    bool bMore = false;

    Data >> sData;
    pCurrBoard = &sPrevBoard;
    iTotalBoards = 0;

    while (    (strlen (sData) > 20)
            && (iTotalBoards < OFFSET)
          )
    {
        if (sData[0] == 'A')
        {
            pNewBoard = (BOARD*) calloc (1, sizeof (BOARD) );
            strcpy (pNewBoard->cBoard, sData);
            pCurrBoard->next = pNewBoard;
            pCurrBoard = pNewBoard;
            iTotalBoards++;
        }

        if (iTotalBoards < OFFSET)
            Data >> sData;
    }

    if (iTotalBoards == OFFSET)
    {
        bMore = true;
    }

    return bMore;
}

---

I ran it through the debugger. It crashes when it tries to return from LoadData the second time. Also, the data loads correctly the first time, but the second call shows nothing being loaded from the file.

My guess is, based on how and where it crashes, that the file is getting automatically closed when I return from LoadData, though I don't understand why.

I'm going to try moving MainData out as a global variable that I don't have to pass to the function and see if that works.

Can someone explain to me why my current version crashes?

I made MainData into a global variable instead of local to main() and now the program seems to be functioning correctly.

I'd still like to know what I was doing wrong before.

Quote:

---

Code:

---

bool LoadData (ifstream Data)

---

---

This is suspicious. It shouldn't be possible to pass an ifstream by value. It should be passed by reference. If passing by value compiles, then your compiler has a problem, and I have no idea what the behavior of this code is.

The program is now working, and the performance improvement is massive. I'm now looking at a different area to improve. Again related to files. This time, the issue is with the output files.

I take 100,000 entries from the input file, process them, sort them and then save them into an intermediate file. Get the next 100,000, process, sort and save into another intermediate file. Repeat until the input file is emptied. Then I need to merge the resulting intermediate files into one large file.

The way I'm currently doing that is to grab the first 2 and merge them, discarding any duplicates. Save the results into another intermediate file and delete those two. Grab the next two and merge. Repeat until all the files are merged down into 1 final file.

This works, but isn't very efficient as it creates an equal number of extra intermediate files which continue to grow in size as the files are condensed together. What I'd prefer to do is create an array of file pointers, open ALL of the original intermediate files at once, then merge them all into one file right then.

What I'm wondering is, what is the limit on the number of files I can have open concurrently? I'm currently working on a Windows XP 5.1, SP3 system. I've tried searching Google, but can't seem to find anything definitive about how many files my single program can open concurrently or whether that number can be adjusted. Anyone know or know where I should look to find out?

Quote:

---

Originally Posted by CornedBee

This is suspicious. It shouldn't be possible to pass an ifstream by value. It should be passed by reference. If passing by value compiles, then your compiler has a problem, and I have no idea what the behavior of this code is.

---

It did compile, and it did run and work through the first call, but not the second.

If it matters, it was compiled with the Microsoft Visual C++ 6.0 compiler.

c++ - Is there a limit on number of open files in Windows - Stack Overflow

Thanks.

I'll go away and stop bugging everyone now.

Again, thanks.

Okay, I'm back again.

Now I'm trying to improve the efficiency of my file merging by merging multiple files at once. But I've again hit a snag. I suspect it is the same issue that CornedBee pointed out cropping up again, but am unsure.

Here is the relevant function which is crashing:

Code:

---

typedef struct inputfiles
{
    char sCurrData[50];
    char sFileName[80];
    ifstream Data;
    struct inputfiles *next;

}INPUTFILES;

typedef INPUTFILES * pInputFiles;

void Debug2 ()
{
    char sData[360] = {0},
        sData2[360] = {0},
        sFile[80];
    int iCurr,
        iDup,
        iLast,
        x,
        iFilesDone,
        iOffset = 7,
        iLevel = 15;
    inputfiles sFileList;
    inputfiles *pFileList = 0,
              *pTemp = 0;
    ofstream Output;

    iCurr = 0;
    iDup = 0;

    sFileList.next = 0;

    for (x = 0; (x - 1) < ( (iOffset - 1) / MERGESIZE); x++)
    {
        for (iLast = 0; (iLast < MERGESIZE) && (iLast + x * MERGESIZE) < iOffset; iLast++)
        {
            pFileList = (pInputFiles)calloc (1, sizeof (inputfiles) );
            sprintf (sFile, "c:\\dev\\connect4\\connect4\\level%d_%d.txt", iLevel, (iLast + 1 + x * MERGESIZE) );
            (pFileList->Data).open (sFile);
            pFileList->Data >> pFileList->sCurrData;
            strcpy (pFileList->sFileName, sFile);

            if (    (sFileList.next == 0)
                || (strcmp (pFileList->sCurrData, sFileList.sCurrData) < 0)
              )
            {
                pFileList->next = sFileList.next;
                sFileList.next = pFileList;
            }
            else
            {
                pTemp = sFileList.next;

                while (    (pTemp->next)
                        && (strcmp (pFileList->sCurrData, pTemp->sCurrData) < 0)
                      )
                {
                    pTemp = pTemp->next;
                }
                pFileList->next = pTemp->next;
                pTemp->next = pFileList;
            }
        }

        if (iLast + x * MERGESIZE < iOffset)
        {
            iOffset++;
            sprintf (sFile, "c:\\dev\\connect4\\connect4\\level%d_%d.txt", iLevel, iOffset);
            Output.open (sFile);
        }
        else
        {
            sprintf (sFile, "c:\\dev\\connect4\\connect4\\level%d.txt", iLevel);
            Output.open (sFile);
        }
        iFilesDone = 0;

        while (sFileList.next)
        {
            if (    (sFileList.next->next)
                && (strcmp (sFileList.next->sCurrData, sFileList.next->next->sCurrData) == 0)
              )
            {
                iDup++;
            }
            else
            {
                Output << sFileList.next->sCurrData << '\n';
                iCurr++;
            }
            sFileList.next->Data >> sFileList.next->sCurrData;
            pTemp = sFileList.next;

            if (strlen (pTemp->sCurrData) < 20)
            {
                sFileList.next = pTemp->next;
                pTemp->Data.close();
//                remove (pTemp->sFileName);
                free (pTemp);
                iFilesDone ++;
                cout << "Completed file " << iFilesDone << " of " << iLast << ".\n";
            }
            else
            {
                pFileList = pTemp;

                while (    (pTemp->next)
                        && (strcmp (pFileList->sCurrData, pTemp->next->sCurrData) < 0)
                      )
                {
                    pTemp = pTemp->next;
                }

                if (pFileList != pTemp)
                {
                    sFileList.next = pFileList->next;
                    pFileList->next = pTemp->next;
                    pTemp->next = pFileList;
                }
            }

//            if (iCurr % 100000 == 0)
            {
                cout << iCurr << " good. " << iDup << " duplicates.\n";
            }
        }

        Output.close();
    }
}

---

I originally was just using an array of structs to hold the file pointers. But I ran into a problem where the first pass worked fine, but every pass after failed to access the data.

The current version shown above crashes with an access violation as soon as I try to open the first file.

Am I just being stupid? Is it something simple like declaring the ifstream pointers inside the struct as pointers instead?

MERGESIZE is currently defined as 3 for debug purposes. Once I get the test cases working, MERGESIZE will be set to 1000 and iOffset and iLevel will be passed in to the final function.

Well, I still don't know what is wrong with that code, but I found a workaround.

First, I tried changing the ifstream to a pointer, but that just caused an Access Violation to occur as soon as I tried to open a file.

I couldn't seem to figure that one out either, so I then decided to try a different approach.

Since the program worked for a single pass (before I switched it to using pointers), I took the for loop out and just made the function do a single pass. Then I put the for loop into a wrapper function which calls the file merging function.

That seems to be working.