-
Notifications
You must be signed in to change notification settings - Fork 893
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
Fix #2229 detect available memory #2236
Conversation
53497af
to
5d88e2c
Compare
One possible critique of the patch is that I haven't wired up the UI to the eventing system. TBH I couldn't be bothered, but I can do that if thats the only issue. However I'm less and less enthralled by that system as time progresses; as we dont support the library as an API, and it's very cumbersome, I'd really like to bring a better system into being. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer to see the use of AtomicBool
otherwise I think it looks good. How confident are you about effective_limits
as a crate for this? I'll defer to your judgement if you say it looks/feels sound.
a3a63c0
to
719ace8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the use of AtomicBool somehow isn't right because the defaulting message is no longer displayed. Either way the tests need checking because they're failing.
f6d5cce
to
f1ff29a
Compare
This doesn't implement streaming IO for low memory situations - we still have the situation that low footprint situations will fail to install, but while it is the case that rustc's memory footprint is lower than our unpack footprint this is probably not urgent to fix, though I will get around to it. Being less aggressive about unpack buffer size though should reduce the number of support tickets from folk in these cases, I hope. We may end up getting tickets from folk with broken ulimit syscalls though, who knows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am really getting to be grumpy about our notification system :D For now, 👍
This doesn't implement streaming IO for low memory situations - we still
have the situation that low footprint situations will fail to install,
but while it is the case that rustc's memory footprint is lower than our
unpack footprint this is probably not urgent to fix, though I will get
around to it.
Being less aggressive about unpack buffer size though should reduce the
number of support tickets from folk in these cases, I hope.
We may end up getting tickets from folk with broken ulimit syscalls
though, who knows.