patterncppMinor

Optimize a parallelized program to optimize train schedules

Submitted by: @import:stackexchange-codereview·Mar 10, 2026·

Viewed 0 times

codereview array cpp c++stackoverflow optimization c++11 multithreading

parallelizedprogramschedulestrainoptimize

Problem

This loops through a vector of vector of A and adds a smaller array to a bigger one at a specified position+random shift. Then it is checked if it is a better solution, and if yes, it is stored, else it is discarded. I have already parallelized it.

Can this be improved to run faster?

I am not sure if a vector> is a good idea, or maybe there is a better memory management to use better cache effects. The struct is currently global and since it is only readable by the threads all threads use the same instance.

I am using C++11 threads.

Description of what the program does:

We have a big array, rand_val, and we have many small arrays, Leistungskurve. We can add these array to rand_val at a fixed position + random shift. We want to find a valid position for each array so the max element in rand_val is minimized. In the array, rand is stated which position we have chosen.

#include 
#include 
#include 
#include 
#include 
#include 

#include 

#include 
#include 
#include 

#include 

#include 
#include 

using namespace std;

struct Teilstrecke {
  int StreckenNr;
  int Fahrzeit;
  int Haltezeit;
  int FruehesteAbfahrt;
  int SpaetesteAbfahrt;
  int Abfahrtsintervall;
  double *Leistungskurve;
};

struct Zug {
  int ZugNr;
  int AnzahlStrecken;
  vector Teilstrecken;
};

vector Zuege;

int Planungsintervall;
int AnzahlZuege;
int AnzahlSchattenzuege;

inline double maxArray(double *arr, int size)
{
  double max = 0;
  int pos = 0;
  for (int i = 0; i  max)
    {
        max = arr[i];
        pos = i;
    }
  }
  return pos;
}

inline void addArray(double *arr1, double *arr2, int size2, int pos)
{
  for (int i = 0; i  threads;
  for (int i = 0; i < 7; i++)
  {
    threads.push_back(thread(wrapper));
  }
  wrapper();
  for (int i = 0; i < 7; i++)
  {
    threads.at(i).join();
  }
}

Profiling

It seems that all work is done in addArray.

Solution

-
Do not use using namespace std as it could cause clashing issues, which will introduce bugs. More info about that here.

-
This is very unorganized:

#include 
#include 
#include 
#include 
#include 
#include 

#include 

#include 
#include 
#include 

#include 

#include 
#include

Steps to improve this:

Make sure there aren't any libraries that you aren't currently using or have repeated.

Make sure you aren't using any C libraries (which end with .h). They should be replaced with the equivalent C++ library so that everything will be in the std namespace.

Sort them in a certain order for organization, such as alphabetically. This will make it easier to locate certain libraries during maintaining.

Assuming every library is still being used, this is what it could look like:

#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include

This is one #include shorter because you've included `

 twice.  Had this list been organized from the beginning, this wouldn't have been likely to happen.

- 
Do not use global variables:

vector Zuege;

int Planungsintervall;
int AnzahlZuege;
int AnzahlSchattenzuege;


They are accessible throughout the entire file or program, and can possibly be modified without your knowledge.  This can greatly hurt maintainability and also introduce bugs.  You should keep them in local scope, such as in a function, and pass them to other functions as needed.  This will make it much easier to keep track of their locality and determine how they're used.

- 
The

inline

 keyword likely won't offer you any benefit here.  The compiler is free to decide what should really be inlined and can ignore any explicit use of it.  It merely serves as a hint.

- 
There is absolutely no need to call

std::rand() when you're using C++11 and have already included . You should utilize this library and ditch std::rand()

 altogether.

Regarding the usage of

std::rand() itself, you don't even call std::srand(), meaning you'll get the same random values with each call to std::rand()

.  Have you not even noticed this?

- 
Why are you mixing

std::vector

s and dynamically-allocated arrays?  Vectors are the same type of array, but much more specialized.  You should ditch all the arrays and just use vectors (or any suitable storage container).  You end up passing these arrays (specifically the pointer to their first respective elements), which can get very nasty in C++, especially because memory-management will now be in your hands.  Just utilize storage containers and let them handle the dirty work for you (which is very necessary, as you fail to deallocate memory with

delete

, causing the whole program to leak memory).  They will also allow you to ditch the size variables since storage containers already keep track of their sizes (accessible via

size()

).

- 
Since you use

argv[1] in main()

 as the only command line argument, I assume that you're only expecting one additional argument.

In order to make sure that this argument is provided, check the value of

argc at the start, before using argv[1]`. If it's less than 2, terminate the program. You may also print an error message to inform the user of this error.

if (argc < 2)
{
    std::cerr << "A filename argument is required!";
    return EXIT_FAILURE;
}

To sum it up:

This is quite broken and messy. I'm sure there are other issues besides the ones I've mentioned, but this should address many of them. Before worrying about optimization, I'd recommend cleaning this up and making sure it runs smoothly and correctly.

Code Snippets

#include <iostream>
#include <string>
#include <vector>
#include <time.h>
#include <cmath>
#include <iterator>

#include <algorithm>

#include <fstream>
#include <stdlib.h>
#include <sstream>

#include <stdlib.h>

#include <thread>
#include <random>

#include <algorithm>
#include <cmath>
#include <cstdlib>
#include <ctime>
#include <fstream>
#include <iostream>
#include <iterator>
#include <random>
#include <sstream>
#include <string>
#include <thread>
#include <vector>

vector<Zug> Zuege;

int Planungsintervall;
int AnzahlZuege;
int AnzahlSchattenzuege;

if (argc < 2)
{
    std::cerr << "A filename argument is required!";
    return EXIT_FAILURE;
}

Context

StackExchange Code Review Q#51837, answer score: 8

Revisions (0)

No revisions yet.