3
\$\begingroup\$

I have a function like this:

BOOL CGameData::GetCheckSum(BYTE o_byObjCheckSum[32], int *o_pnFileSize, char* pFilePath)
{
memset(o_byObjCheckSum, 0x00, 32);
*o_pnFileSize   = 0;

UINT uiCheckSum = 0;
if(strlen(pFilePath) <=0)
{
return FALSE;
}
FILE *fp;
fp=fopen(pFilePath, "rb");
if(NULL == fp)
{
    return FALSE;
}
fseek( fp, 0L, SEEK_END );
long lFileSize = ftell( fp );
*o_pnFileSize = lFileSize;
fseek( fp, 0L, SEEK_SET );  

BYTE *pFileData = new BYTE [lFileSize];
fread(pFileData, lFileSize, 1, fp); 
sha256_encode(pFileData, lFileSize, o_byObjCheckSum);

fclose(fp);
delete [] pFileData;

return TRUE;
}

How can I make this faster?

If it will help, this function is called here:

    STRNCPY_MEMSET(szResDirectoryPath, RESOBJ_DIRECTORY_PATH, MAX_PATH);
    if(FALSE == GetAllFileNameList(&vectFileNameList, szResDirectoryPath))
    {
        g_pFieldGlobal->WriteSystemLogEX(TRUE, "[ERROR] LoadResObjCheckList_ error !!, Directory(%s)\r\n", szResDirectoryPath);
        return FALSE;
    }

    nCnt = vectFileNameList.size(); 
    for(i=0; i < nCnt; i++)
    {
        SRESOBJ_CHECKSUM    resObjCheckSum;
        CGameData           tmMapGameData;
        char                szFileFullPath[MAX_PATH];
        MEMSET_ZERO(&resObjCheckSum, sizeof(resObjCheckSum));

        STRNCPY_MEMSET(resObjCheckSum.szResObjFileName, vectFileNameList[i].c_str(), SIZE_MAX_RESOBJ_FILE_NAME);
        _strupr(resObjCheckSum.szResObjFileName);
        sprintf(szFileFullPath, "%s/%s", szResDirectoryPath, resObjCheckSum.szResObjFileName);

        if(tmMapGameData.GetCheckSum(resObjCheckSum.byDigest, &resObjCheckSum.nFileSize, szFileFullPath))
        {
            m_mapResObjCheckSumList.insert(pair<string,SRESOBJ_CHECKSUM>(resObjCheckSum.szResObjFileName, resObjCheckSum));
        }
    }
\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to Code Review! I hope you get some great answers! \$\endgroup\$ – Phrancis Feb 23 '15 at 17:25
  • \$\begingroup\$ Have you profiled your code? If so, is there actually a bottleneck in this function? \$\endgroup\$ – Gnawme Feb 23 '15 at 17:53
  • \$\begingroup\$ yes i have profiled, actually this is part of gameserver code. It loads a lot of other things before and after resource objects loading. So i want to make loading objects faster because of fread and sha256_process during around 2 minutes. So theres is around 5300 files to read and create checksums. Thats why i want to make this faster \$\endgroup\$ – GameDev Feb 23 '15 at 18:08
  • \$\begingroup\$ What implementation of sha256 are you using ? \$\endgroup\$ – Loufylouf Feb 23 '15 at 19:01
  • \$\begingroup\$ * FIPS-180-2 compliant SHA-256 implementation * * Copyright (C) 2001-2003 Christophe Devine \$\endgroup\$ – GameDev Feb 23 '15 at 19:33
3
\$\begingroup\$

In general, the file input takes the most amount of processing time.

Your CRC algorithm may also be consuming a lot of the processing time.

Here are some suggested optimizations:

  • Don't clear the checksum. The CRC will write to it anyway. This is a wasted function call.
  • The strlen function searches the string to determine length. Use std::string instead, because it maintains the length of the string and can return the string length faster.
  • Don't read the entire file into memory. Pick a chunk size and use that. A seek to the end of file may consume a lot of time. If you must get the file size, use an OS API that will return the size of the file; hopefully it will be faster than seeking to the end of the file.
  • If your OS supports memory mapped files, you may want want to use this. The memory mapped files allows the OS to handle reading the file into memory.
  • Don't keep allocating from the heap. Create a large array once and use that. Memory allocation may become a bottleneck when the memory becomes fragmented.
  • Turn on compiler optimizations to highest level.
  • Try creating multiple threads for processing. For example, you could spawn two threads, the main program would pass different filenames to the threads as they complete their processing.
  • Reduce the number of branches; branches may force instruction pipeline reloading, which wastes time that could be spent processing data. Research "loop unrolling".
  • Optimize data to fit into the cache. Reloading of the cache for items outside the cache wastes time.
  • If you can, do whatever it takes to keep the hard drive spinning. There is an overhead associated with starting up the hard drive motors. Keeping the drive spinning reduces the need to restart the motors. See also "double buffering".
  • Optimize the CRC by performing it in parallel, if possible.
\$\endgroup\$
  • \$\begingroup\$ i did smth like this: std::string pFP (pFilePath); if(pFP.length() <= 0) { and seems to be faster a bit \$\endgroup\$ – GameDev Feb 24 '15 at 15:10
1
\$\begingroup\$

It wouldn't help the code run faster, but it would certainly be easier to read if you used proper indentation:

if(strlen(pFilePath) <=0)
{
return FALSE;
}

should be

if(strlen(pFilePath) <= 0) {
    return FALSE;
}

Notice that I also added a missing space around the <= operator and shifted the brace up next to the end of the if statement according to common C++ style.

Also, why are you using capital TRUE and FALSE variables? In C++, those variables are lowercase, did you do something like #define TRUE true? If so, it doesn't add any meaning to the code, so you shouldn't.

None of the really good C++ I've seen ever lines variable declarations up like this:

SRESOBJ_CHECKSUM    resObjCheckSum;
CGameData           tmMapGameData;
char                szFileFullPath[MAX_PATH];

Just put one space there.

I don't know C++ at this level, so I will leave optimizing it to the experts. Hope you get some good reviews on that.

\$\endgroup\$
  • \$\begingroup\$ thanks for your answer but thats only cosmetics, i need to make function faster with reading files and inserting their created checksums to list. This function is too slow for serious game project \$\endgroup\$ – GameDev Feb 23 '15 at 17:58
  • \$\begingroup\$ I understand, but having well-written code is important too. It is easier to read, debug, and improve well-written code. \$\endgroup\$ – Hosch250 Feb 23 '15 at 18:02
1
\$\begingroup\$

A new/delete sequence is quite suspicious. Having the whole file in memory could be very demanding on the system, especially if the file is large (think of cache misses). Computing SHA in a streaming manner:

sha_init(ctx);
while ((len = read_chunk_of_file(buffer)) > 0) {
    sha_update(ctx, buffer, len);
sha_final(digest, ctx);

relaxes requirements, and should lead to better performance. Of course, only large files would benefit.

\$\endgroup\$
  • \$\begingroup\$ i have to load a lot of files but small sizes - from 1 to 3 MB and their count its over 5300 \$\endgroup\$ – GameDev Feb 23 '15 at 20:15

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.