memory/references issue

[alexas]

I am again here with my struggle with pointers/references.
I simplified everything to the two simple functions that compiles - but does not work as intended (the actual function should run on parallelism with pragma, etc so more complicated). I can see clearly that references are showing to the wrong point of memory, i.e. the wrong data are being put and so the second function always return true. Could anyone advise please?

bool
StructureGenerator::_check_polymer_intersection (std::vector<Polymer>& polymers,const Polymer& polymer)
{
    bool check = false;
    for (int32_t i = 0; i < polymers.size(); i++)
    {
        if (check) break;
        if (polymer.polymer_intersecting(polymers[i]))
        {
            check = true;
        }
    }
    return check;
}
 
 
bool
Polymer::polymer_intersecting (Polymer& other) const
{
    const auto nom = number_of_monomers();
    const auto onom = other.number_of_monomers();

    auto bd1 = allocate_bd();
    auto bd2 = allocate_bd();

    for (int32_t i = 0; i < nom; i++)
    {
        get_mcoord(i, &bd1);
        for (int32_t j = 0; j < onom; j++)
        {
            other.get_mcoord(j, &bd2);
            if (check_gjk_intersection(&bd1, &bd2))
            {
                return true;
            }
        }
    }
    free_bd(&bd1);
    free_bd(&bd2);
    return false;
}
 

[TheIdeasMan]

Hi,

Please show the definition of allocate_bd()

As mentioned in the other topic by coder777, does it return a pointer? If so, should lines 28, 31, 32 dereference (or ->) rather than addressof ?

If allocate_bd() allocates memory, where is that memory initialised to some values?

Can you post a small complete program that shows the issue?

What happened when you debugged these functions? --> wrong memory address --> where / how is memory allocated? Any other wrong values during debugging?

Rather than start a new topic, just keep the original one going - it's about the same subject.

Edit:

Hopefully the functions were called with the correct arguments :+) Sounds silly, but it can happen.

[salem c]

Well the first problem with the return true is that you're leaking the bd allocations by not calling free_bd.

> the actual function should run on parallelism with pragma, etc so more complicated
Before you even think about that, you need to make sure the code is fully functional on small (but representative) data sets as normal sequential code.
You're already struggling with debugging sequential code, premature parallelism is just making it impossible for you to reason about anything you observe.

https://www.cplusplus.com/forum/beginner/278284/
Your level of parallelism seems far too detailed.
polymer_intersecting doesn't need any parallelism, it just needs to be re-entrant.
That is, it depends only on it's parameters and internal variables, and doesn't write to any global data.
Allocating memory is fine, so long as you release it.

If you try to micro-manage this at too low a level, you're just going to thrash the system with expensive (compared to the actual amount of work done) context switches.

Then I would add just one #pragma omp parallel to the for loop in _check_polymer_intersection
That is, each parallel thread is responsible for checking an entire polymer for an intersection.
Then you measure the performance to see how much of an improvement (if any).

Give each thread a useful amount of work to do, then you only pay the overhead price of thread management once.

[alexas]

Thanks for your input. I know about the pragma, but please do not relate that to the previous post. The same function goes sometimes as serial, sometimes as parallel depending on the certain cases; every case is carefully tested and the most optimal is selected. In this particular case the polymer intersecting does not need parallelism as it was correctly noted, only the check_polymer goes parallel.

allocate_bd allocates the memory:

inline struct bd
allocate_bd()
{
    struct bd bd{};
    bd.numpoints = 8;
    bd.coord = (double **) malloc(8 * sizeof(double *));
    for (int32_t i = 0; i < 8; i++)
    {
        bd.coord[i] = (double *) malloc(3 * sizeof(double));
    }
    return bd;
}

then get mcoord puts that value to the address:

void
Polymer::get_mcoord(int32_t index, struct bd *bd) const
{
}

I am sorry it is not possible to show the fully working code as there are many related functions. However, all of them works correctly. Polymer_intersecting works correctly too, it manages to check the polymer, but the problem is that it seems that is taking the data from the wrong memory place, therefore always return true.

I added free_bd after the return true but that does not solve the problem.

[TheIdeasMan]

Hi,

So your allocate_bd function is returning a local variable -> it goes out of scope straight away? Should you pass bd by reference to allocate_bd ? If it is an inline function in terms of C++17, the local bd would need to be static?

I don't know much about OpenMP, but I noticed in the API that it has it's own allocate directives, and memory spaces, can you use them instead? I was looking at version 5.1. Which version are you using?

alexas wrote:

I am sorry it is not possible to show the fully working code as there are many related functions.

But can you provide a program with just the code in your first post, with a main function and some data to call the 2 functions? Sufficient that we can compile / debug it.

[coder777]

but the problem is that it seems that is taking the data from the wrong memory place

What makes you think so?
I would suggest that you output (cout) the obtained values.

In allocate_bd() you leave the coord[...] uninitialized. Maybe this is a problem maybe not. So a calloc(...) would be better.

Why do you dynamically alloc the memory at all? They are fixed size.
In a c++ program you should use new anyway:

bd.coord[i] = new double[3]{};

The return on line 34 will leave a memory leak.

[TheIdeasMan]

coder777 wrote:

Why do you dynamically alloc the memory at all? They are fixed size.
In a c++ program you should use new anyway:

Going from what lastchance wrote, why can't the OP use std::vector? Maybe there is an effort to wring every bit of performance by using a C style of coding in some of the functions, but they are still members of a C++ class? Interesting to see that a project with a large scale of data on a supercomputer is using C++, and not FORTRAN. Although I did read recently that it depends on what one is doing to choose between C++ and FORTRAN.


https://www.cplusplus.com/forum/general/278405/#msg1201979

lastchance wrote:

@JamieAl,
Why don't you simply use std::vector rather than all those dynamic arrays? Then you will never have to use a malloc or a free (which will reduce the line count in your code by 20% or so to start with, and the likelihood of bugs by a similar amount).

Your library routines may not directly use vectors, but they do use flat 1-d arrays, so if V is a vector then you can simply pass a reference to its data buffer: V.data(). It will still work.

[alexas]

as I told before, I have a library where I am forced to use the pointers and did not know how to avoid that.

So I have the following structure:

struct bd {
    int numpoints;    /**< Number of points defining the body.            */
    double **coord;  /** points' coordinates. */
};

and I decided very unfortunately to go alongside that and using such structure which is the real pain for my skills (yet it actually works very fast, I must admit). So question: how exactly I can avoid this and use the std::vector and point its 8 values to **coord?

inline bool
check_gjk_intersection(struct bd *bd1, struct bd *bd2)
{
    dd = gjk(*bd1, *bd2, &s);

    if (dd < 0.0005)  return true;

    return false;
}


inline struct bd
allocate_bd()
{
    struct bd bd{};

    bd.numpoints = 8;

    bd.coord = (double **) malloc(8 * sizeof(double *));

    for (int32_t i = 0; i < 8; i++)
    {
        bd.coord[i] = (double *) malloc(3 * sizeof(double));
    }
    return bd;
}


inline void
free_bd(struct bd* bd)
{
    for (int32_t i = 0; i < 8; i++)
    {
        free(bd->coord[i]);
    }
    free(bd->coord);
}

[jonnin]

I don't know if its worth it, but this is A way to do that, and as a side effect, you get a 1-d container you can iterate when you want to just touch each item .. a little more efficient for some operations.

int main()
{	
	vector<double> f2(50);  //50 is rows * cols, example will be a 5x10 2d thing. 
	std::iota(f2.begin(), f2.end(), 1);	 //just some data 1-50 stuffed into it
	
	vector <double*> bar(5);  //create the second dimension. 
	for(int i = 0; i < 5; i++) //and populate it. 
	bar[i] = &f2[i*10];
    
	double **d2 = &bar[0];   //now its a double pointer... 
	for(int i = 0; i < 5; i++) //proof of concept: print f2 as if it were a double**
	{
		for(int j = 0; j < 10; j++)
			cout << d2[i][j] <<" ";
		cout << endl;
	}
	
}

note that vectors, like arrays, are solid blocks of memory. the above is exploiting that, heavily (line 11 would be a total trainwreck if not for this).
note that if you cause EITHER vector to resize itself by push-back or whatever commands, it will invalidate ALL the pointers under the umbrella (eg if you resize f2 everything is invalid). Fixed size vectors are fine to take pointers out of but do not want to go there with constantly resizing ones.

[alexas]

thank you very much for your help. I will certainly try to rewrite the code and test this. Thanks,

[jonnin]

it should work as-is with some includes. Test it first like it is, and verify that you believe that it works without any tampering. Then you have a baseline if your rewrite has issues. my junk code file has excess but here is what it has in it:

#include <iostream>
#include <iomanip>
#include <fstream>
#include <string>
#include <fstream>
#include<vector>
#include <numeric>

with that and namespace std (bad me!) it should run as-is.

if you run into trouble let me know. Its kind of hackery, but I believe it is well defined hackery. If someone sees a violation (undefined behavior), I am open to reform.

I always forget what all valarray can do. Its possible you can slice something up with less hands-on using that. There may be some other cleaner way to do what I did.. I tend to get barbaric when I have a problem like this, so basically I took your C++ objects and beat them back into C with a big hammer.