Smaller closure #4385
Smaller closure #4385
Conversation
rts/idris_rts.c
Outdated
| #define STATIC_ASSERT(COND,MSG) typedef char static_assertion_##MSG[(COND)?1:-1] | ||
|
|
||
|
|
||
| STATIC_ASSERT(sizeof(Closure) == 16, sizeofClosure); |
There was a problem hiding this comment.
This could be false on 32-bit systems depending on how double is aligned.
There was a problem hiding this comment.
I guess a 32-bit "test_c" job in the CI would be nice... I have a 32-bit Nix system at work, I can try there anyway.
|
Thanks for the PR! |
|
I’ll take a serious look at it after work. Please try to test a 32-bit build also. |
|
I don't expect this to be merged yet, I opened it to get some feedback on whether this is worth pursuing. I think I can make some further improvements to reduce memory usage even more (and maybe simplify it at the same time). Please, wait before doing a real review. |
|
I'm trying an alternative approach using a header instead of a union at https://github.com/jacereda/Idris-dev/tree/smaller-closure-3 and the result seems promising. Should I open a separate PR or merge into this branch? |
|
Better to keep it in one PR as only one of the ways will get in. I don't think there should be bitfields. It makes it a pain to access from other languages than C, and it would be nice if one could mess with Idris internals from Idris code. Also, we don't have to reserve 56 bits for size, let's say that people who want to allocate more than 4GB have to use a pointer. |
|
@melted, I think this is ready. I have a couple of branches on top of this one that make further improvements like reducing Bits8/Bits16 to 8 bytes and improving support for developing on NixOS/Nixpkgs, but I guess it would be better to go in small steps. Also, I guess I should have opened a separate PR for 11130b1, since it's a bug. The 32-bit test suite is still broken, but that's also the case in master. |
config.mk
Outdated
| @@ -14,7 +14,7 @@ RANLIB ?=ranlib | |||
| CABAL :=cabal | |||
| # IDRIS_ENABLE_STATS should not be set in final release | |||
| # Any flags defined here which alter the RTS API must also be added to src/IRTS/CodegenC.hs | |||
| CFLAGS :=-O2 -Wall -std=c99 -D_POSIX_C_SOURCE=200809L -DHAS_PTHREAD -DIDRIS_ENABLE_STATS $(CFLAGS) | |||
| CFLAGS :=-O2 -Wall -g -std=c99 -D_POSIX_C_SOURCE=200809L -DHAS_PTHREAD -DIDRIS_ENABLE_STATS $(CFLAGS) | |||
rts/idris_gc.c
Outdated
| heap_item->info.str_offset->str | ||
| = copy(vm, heap_item->info.str_offset->str); | ||
| break; | ||
| case CT_CON: { |
There was a problem hiding this comment.
uneven indentation in the cases.
There was a problem hiding this comment.
Can I add a .clang-format file that tries to be consistent with current formatting?
rts/idris_gmp.c
Outdated
| } | ||
|
|
||
| VAL idris_bigAnd(VM* vm, VAL x, VAL y) { | ||
| if (ISINT(x) && ISINT(y)) { | ||
| return INTOP(&, x, y); | ||
| } else { | ||
| return bigAnd(vm, GETBIG(vm, x), GETBIG(vm, y)); | ||
| return bigAnd(vm, (VAL)GETBIG(vm, x), (VAL)GETBIG(vm, y)); |
There was a problem hiding this comment.
GETBIG is defined with a cast to VAL already.
Further occurences below.
rts/idris_rts.h
Outdated
| typedef struct String { | ||
| Hdr hdr; | ||
| size_t slen; | ||
| #define _null hdr.u8 |
There was a problem hiding this comment.
Remove this and substitute hdr.u8 in the use sites and put in a comment that String uses the hdr.u8 field for null.
rts/idris_rts.h
Outdated
| typedef struct Con { | ||
| Hdr hdr; | ||
| uint32_t tag; | ||
| uint32_t arity; |
There was a problem hiding this comment.
Arity could use one of the fields in the header. That would save four bytes on 32-bit.
There was a problem hiding this comment.
Not now, since the aligned() function is now requiring 8 bytes alignment, but aligned() could as well align to sizeof(intptr_t). I'll change that.
|
Let's merge this. I'll note it in the change log. Thanks for the contribution! |
Looks like something like this could be useful to reduce GC frequency. Thoughts?