Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introcuding safe_malloc and safe_realloc macros: fixes #480 #481

Merged
merged 5 commits into from
Feb 8, 2016

Conversation

unoebauer
Copy link
Contributor

Simple safe_malloc and safe_realloc macros are introduced. These check for returned NULL pointers in the allocation process and stop the calculation in these cases.

  • introduce safe_malloc and safe_realloc macros
  • replace memory operations with safe counterparts
  • test it:
    • tardis_example
    • tardis_example with OMP
    • tardis_example with v-logging
    • tardis_example with OMP and v-logging

* introduced safe_malloc and safe_realloc macros
* these abort the simulation if a NULL pointer is returned in the
  allocation or reallocation process
* replaced mallocs and reallocs with safe counterparts
@yeganer
Copy link
Contributor

yeganer commented Feb 8, 2016

When i was looking at the C code I noticed several uses of malloc in combination with memcopy to zero out the allocated space. I think we should replace those with calloc which uses the operating system to do that.

Another thing i noticed is the exponential behavior of the allocation for the virtual packets. using a good guess for the 1st allocation we should only need to realloc in special cases. In these cases we should only extend the array by no_of_packets and not double it.

*/
static inline void* safe_malloc(size_t size) {
void *mem = malloc(size);
if (mem == NULL && size != 0) FAIL();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the point of the FAIL macro?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, we could get rid of it

@unoebauer
Copy link
Contributor Author

@orbitfold, @yeganer, @wkerzendorf Unit tests needed for the macros?

@wkerzendorf
Copy link
Member

@yeganer we have no general good guess for then number of virtual packets. @orbitfold mirrored pythons way allocating memory for lists.

@wkerzendorf
Copy link
Member

@unoebauer would be good - but I don't know if that is easy.

@unoebauer
Copy link
Contributor Author

@yeganer: you kneed to know the number of real packets (trivial - set in the config), the number of virtual packets spawned each time the virtual packets are generated (trivial - set in the config) and finally the number of events triggering the generation of v-packets (very hard - each time a real packet is launched -> trivial - and each time a real packet interacts -> very hard).

@yeganer
Copy link
Contributor

yeganer commented Feb 8, 2016

@unoebauer If I interpret this correctly we have

no_virt_packets = no_of_packets * virt_packet_count * average_spawn_per_packet

So all we have to do is generate some statistics, ceil the result and every time it runs out of space add another no_of_packets.

@wkerzendorf
Copy link
Member

@yeganer what's wrong with the current approach? I doubt that it causes any problems. I mean even with 10 million floats this is still only 80 Mb in memory.

@unoebauer
Copy link
Contributor Author

@yeganer Let's not overdo it - --with-vpacket-logging is a diagnostics tool. It won't be active in normal runs. So no need to invest too much time into making the memory allocation process very sophisticated.

Moreover, doing statistics on this would be a lengthy process because there's a huge parameter space to be covered.

BTW, an estimate for the number of virtual packets would look like:

no_virt_packets = no_real_packets * virt_packets_per_event + no_real_packets * virt_packets * average_no_real_packet_interactions

@unoebauer
Copy link
Contributor Author

@yeganer, @orbitfold, @wkerzendorf
Given the simplicity of the macros, I'd not include unit tests. Tardis example runs fine (see milestones), Travis is happy so I'd merge it....

@yeganer
Copy link
Contributor

yeganer commented Feb 8, 2016

@unoebauer Yeah you are right. This isn't needed. I just had the segfault issue in the back of my had and was wondering if this was connected. After doing the math, I agree this shouldn't be connected.

Update:

I'm fine with merging. Unit tests shouldn't be needed. I assume you DID test the macros in a local environment?
I think we should add an error message.

wkerzendorf added a commit that referenced this pull request Feb 8, 2016
Introcuding safe_malloc and safe_realloc macros: fixes #480
@wkerzendorf wkerzendorf merged commit f50695c into tardis-sn:master Feb 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants