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

Define gc counted allocator functions with libc API #12034

Merged
merged 1 commit into from
Jul 13, 2015
Merged

Conversation

andreasnoack
Copy link
Member

...and use them in CHOLMOD wrappers. This makes the memory consumption from SuiteSparse visible to our GC. Right now SuiteSparse object looks tiny to the GC even though they are huge and in consequence much memory is left allocated when it could be freed.

In particular, I'd like comments on the gc.c part of this as I haven't touched that part of the code base before.

{
size_t *p = (size_t *)jl_gc_counted_malloc(sz + sizeof(size_t));
p[0] = sz;
return (void *)(p + 1);
Copy link
Member

Choose a reason for hiding this comment

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

this needs to add 16 bytes, to keep p aligned

Copy link
Contributor

Choose a reason for hiding this comment

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

does malloc has any alignment guarantee beyond sizeof(void*) ?

Copy link
Member

Choose a reason for hiding this comment

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

yes, on almost all modern platforms, it guarantees 16-byte alignment

edit: if size > 16

Copy link
Contributor

Choose a reason for hiding this comment

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

we might as well have this be 16 aligned then. I would think that libraries actually needing such alignment would be defensive about it, but let's not be greedy for 8 bytes (aka the Jameson way :p )

@yuyichao
Copy link
Contributor

yuyichao commented Jul 6, 2015

Actually I just come across malloc_usable_size in GLibC, Can we use it on systems where it's available?

@andreasnoack
Copy link
Member Author

Thanks for the comments. Here is a new attempt.

@vtjnash
Copy link
Member

vtjnash commented Jul 6, 2015

Actually I just come across malloc_usable_size in GLibC, Can we use it on systems where it's available?

it's not portable, although something similar is available on most systems

@ScottPJones
Copy link
Contributor

There are aligned_alloc and posix_memalign, depending on platform, that can give you 16 byte alignment.

@yuyichao
Copy link
Contributor

yuyichao commented Jul 6, 2015

it's not portable, although something similar is available on most systems

Depend on what's the allocation pattern of cholmod I'm just wondering how much memory we'll save by not storing the same information (size) twice.

Edit: and whether it's worth writing a non-portable optimization for it.

@carnaval
Copy link
Contributor

carnaval commented Jul 6, 2015

Well it's a non breaking change to go back and use platform dependent functions, so let's merge this first. Most C code that cares about perf wont use malloc for small things anyway.

@yuyichao
Copy link
Contributor

yuyichao commented Jul 6, 2015

Agree. (especially the non-breaking improvement part).

@kshyatt kshyatt added sparse Sparse arrays GC Garbage collector labels Jul 6, 2015
@andreasnoack andreasnoack force-pushed the anj/gc branch 5 times, most recently from c63497b to 7f74c23 Compare July 10, 2015 17:38
@andreasnoack
Copy link
Member Author

Tests are passing now. Had to update the .travis.yml. @vtjnash would you take a look at the changes to jl_gc.c and see if they seem right? Everything should be 16 byte aligned now.

@vtjnash
Copy link
Member

vtjnash commented Jul 13, 2015

lgtm

andreasnoack added a commit that referenced this pull request Jul 13, 2015
Define gc counted allocator functions with libc API
@andreasnoack andreasnoack merged commit 2b0faa1 into master Jul 13, 2015
@andreasnoack andreasnoack deleted the anj/gc branch July 13, 2015 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GC Garbage collector sparse Sparse arrays
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants