-
Notifications
You must be signed in to change notification settings - Fork 996
runtime (gc): add lightweight list GC
#1193
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
Conversation
bbc2bf5 to
5abcde9
Compare
|
This looks like an interesting small GC. I think it may be useful for more than just the AVR. |
|
Okay I will do that once i finish the other gc. |
|
Did some testing with this branch, and it really works well and saves a lot of size on Arduino Uno. Current With this branch: Any reason we should not merge it in now, @jaddr2line and @aykevl ? |
|
Yeah, there is still a bit more to do (see aykevl's comment) |
|
Also, I think it's a good idea to rename the conservative GC. Right now it's a bit too broad (all real GCs implemented are conservative). Maybe |
|
This seems to have run into issues on |
|
It seems like the issue was fixed. After rebasing to try to be able to use the stack depth checker, the issue disappeared. |
|
Nevermind, the issue is still here I just gave a wrong command. |
|
It looks like the allocation of the task frame is being collected. |
list GC
|
I tried to use this but I got an error on AVR: Additionally I tried running |
|
The Not sure why AVR is broken now, it worked before i pushed the commit. I will try to figure out what broke. |
|
No, actually the arduino works for me, it turns out I was just using an older version of the tinygo compiler. You need to |
|
Ok I was able to replicate the atomic hardfault and will look into it. |
|
Okay so I think figured it out now. Apparently we are generating a ldrexd instruction somehow, which from what I can tell is not supported on cortex-m3. |
|
This isn't 8-byte aligned now with this memory manager. I attempted to fix this by adjusting the GC to use the alignment of a uint64. . . but the alignment is actually 8-bytes and this is correct because cortex-m3 does not have that instruction. It seems that QEMU does not replicate this accurately, and runs the instruction anyway. Basically, we are hitting an alignment error in an unsupported instruction. . . |
|
This should not have ever worked. |
Ah thanks, that's it. Now it works for me (sort of): |
|
Yeah I think i broke some stuff when generalizing. . . |
|
Wait no everything is working edit: actually why is this happening |
|
Ok no the false positive rate is just really really bad. |
|
With 16 KiB you should really be expecting like a 25% false positive rate, but it only needs to happen once so it is actually well past a 90% false positive pinning rate. |
|
Basically, any byte less than 64 with another byte after it points into the heap. . . |
|
Okay it looks like I am going to be turning this into a precise GC instead |
|
Just looking at this PR again. What is the status with it @niaow ? |
|
It works on ARM, but on AVR the false positive rate is too high for it to be usable. I started turning it into a precise collector but have not had much time to work on it. |
|
ok i pushed what i have of the precise gc |
Looks like the conflicts have to be resolved to run CI on Windows. |
|
Huh that's strange. We're not using AppVeyor. But I did try using that service in the past and don't think I ever disabled it, so maybe it needs to be disabled. But I agree, this PR needs to be rebased on the dev branch. |
I was planning to wait until this got a bit further along, but I guess I can do that sooner.
This PR isn't actually close to ready - this currently causes LLVM's DCE pass to crash under many conditions. |
ec238c6 to
ada4e12
Compare
|
After this most recent commit, a large amount of code is able to build without needing scannable allocations. |
|
Hmm. . . so i changed |
635e57d to
110a94c
Compare
|
@aykevl I think this is finally actually ready for review. |
|
The CI failures appear to be unrelated. . . |
|
I should probably rebase this. . . |
|
After rebasing there seems to be a miscompile/UB of |
|
Finally getting back around to this. I think I am going to split this PR so that we can merge the |

This saves about a kilobyte of flash, and attempts to address the AVR size increase issue in #1181. Similar to #1181 and the avr-libc allocator, this is a best-fit allocator.