Introduction; suggestion to improve the code

[Claus Volko]

Hi there,

I'm a senior software engineer who also has clue about biology/medicine and is a specialist for algorithm design and code optimization.

I've recently had a talk with ChatGPT about how I might contribute to medical research in my spare time. ChatGPT gave me the advice to contribute to COPASI, as there are supposedly many code passages that could be optimized.

I've only taken a short glance into the code of COPASI so far, but I immediately found a code passage that could be optimized:

// static
void CMathContainer::createRelocation(const size_t & newSize, const size_t & oldSize,
                                      CMath::sRelocate & currentRelocation,
                                      std::vector< CMath::sRelocate > & relocations,
                                      const bool & modifiedAtEnd)
{
  if (newSize != oldSize)
    {
      // Size modifications are made at the end of the current section
      if (modifiedAtEnd)
        {
          currentRelocation.pValueEnd = currentRelocation.pValueEnd + std::min(newSize, oldSize);
          currentRelocation.pObjectEnd = currentRelocation.pObjectEnd + std::min(newSize, oldSize);

          if (currentRelocation.pValueStart != currentRelocation.pValueEnd)
            {
              relocations.push_back(currentRelocation);
            }

          currentRelocation.pValueStart = currentRelocation.pValueEnd - std::min(newSize, oldSize) + oldSize;
          currentRelocation.pObjectStart = currentRelocation.pObjectEnd - std::min(newSize, oldSize) + oldSize;
          currentRelocation.pValueEnd = currentRelocation.pValueStart;
          currentRelocation.pObjectEnd = currentRelocation.pObjectStart;
          currentRelocation.offset += (newSize - oldSize);
        }
      // Size modifications are made at the beginning of the current section
      else
        {
          if (currentRelocation.pValueStart != currentRelocation.pValueEnd)
            {
              relocations.push_back(currentRelocation);
            }

          currentRelocation.pValueEnd += oldSize;
          currentRelocation.pObjectEnd += oldSize;
          currentRelocation.pValueStart = currentRelocation.pValueEnd - std::min(newSize, oldSize);
          currentRelocation.pObjectStart = currentRelocation.pObjectEnd - std::min(newSize, oldSize);
          currentRelocation.offset += (newSize - oldSize);
        }
    }
  else if (newSize > 0)
    {
      currentRelocation.pValueEnd += newSize;
      currentRelocation.pObjectEnd += newSize;
    }
}

In this code, std::min(newSize, oldSize) is evaluated twice or even four times, although it would suffice to evaluate it only once as the values of newSize and oldSize don't change. I'd propose the following solution:

// static
void CMathContainer::createRelocation(const size_t & newSize, const size_t & oldSize,
                                      CMath::sRelocate & currentRelocation,
                                      std::vector< CMath::sRelocate > & relocations,
                                      const bool & modifiedAtEnd)
{
  if (newSize != oldSize)
    {

      size_t minSize =  std::min(newSize, oldSize);

      // Size modifications are made at the end of the current section
      if (modifiedAtEnd)
        {
          currentRelocation.pValueEnd = currentRelocation.pValueEnd +  minSize ;
          currentRelocation.pObjectEnd = currentRelocation.pObjectEnd +  minSize ;

          if (currentRelocation.pValueStart != currentRelocation.pValueEnd)
            {
              relocations.push_back(currentRelocation);
            }

          currentRelocation.pValueStart = currentRelocation.pValueEnd -  minSize  + oldSize;
          currentRelocation.pObjectStart = currentRelocation.pObjectEnd -  minSize  + oldSize;
          currentRelocation.pValueEnd = currentRelocation.pValueStart;
          currentRelocation.pObjectEnd = currentRelocation.pObjectStart;
          currentRelocation.offset += (newSize - oldSize);
        }
      // Size modifications are made at the beginning of the current section
      else
        {
          if (currentRelocation.pValueStart != currentRelocation.pValueEnd)
            {
              relocations.push_back(currentRelocation);
            }

          currentRelocation.pValueEnd += oldSize;
          currentRelocation.pObjectEnd += oldSize;
          currentRelocation.pValueStart = currentRelocation.pValueEnd -  minSize ;
          currentRelocation.pObjectStart = currentRelocation.pObjectEnd -  minSize ;
          currentRelocation.offset += (newSize - oldSize);
        }
    }
  else if (newSize > 0)
    {
      currentRelocation.pValueEnd += newSize;
      currentRelocation.pObjectEnd += newSize;
    }
}

Cheers

Claus

[juergen]

Hi Claus,

Thank you for your contribution. Copasi has a large and complex codebase by now, and surely there are many instance where the code could be optimised slightly. In the case you brought up, the compiler might even be able to opimise this, so that std::min is only called once.

Best wishes,

Juergen

[Claus Volko]

Hi Juergen,

I'm aware of the optimization capabilities of the compiler, but in this case it isn't so trivial because in theory, the values could be modified by another task running in parallel. I made the assumption that this doesn't happen. The compiler wouldn't make this assumption.

Cheers

Claus

--
You received this message because you are subscribed to the Google Groups "COPASI User Forum" group.
To unsubscribe from this group and stop receiving emails from it, send an email to copasi-user-fo...@googlegroups.com.
To view this discussion visit https://groups.google.com/d/msgid/copasi-user-forum/2baeb015-dc05-4137-b538-44cc5363e88dn%40googlegroups.com.

--

http://www.cdvolko.net/

[juergen]

Hi Claus,

You are certainly right. One would have to check the compiled code really to be able to say what exactly happens here.

Thanks,

Juergen

[Frank Bergmann]

Thank you so much Claus, 

we are in the middle of preparing for a new release, and merge your pull request once it is out. 

best

Frank

[Claus Volko]

That's great to hear! Thank you very much!

http://www.cdvolko.net/

To view this discussion visit https://groups.google.com/d/msgid/copasi-user-forum/71767003-2afb-4210-873a-8c1c32b1fcb9n%40googlegroups.com.