Foiling CG #43
Foiling CG #43
Conversation
… they are off the stack.
|
I'm not so happy about using |
|
If you would prefer I can change to using RB_GC_GUARD. Let me explain my reasons first though. There is a good article on memory management for ruby at https://ruby-hacking-guide.github.io/gc.html. It does into more detail than most car about. Search for volatile to get skip a lot of the details. More than once when Oj was growing up I had complaint about stack use for deeply nested objects using multiple threads with smaller stacks. I went spend some time trying to keep the stack as tight as I could for each iteration. The RB_GC_GUARD was one of the places I could get rid of an extra expansion of the stack. Pretty minor but every bit helped. Since coming to understand what Ruby is doing with the GC I've always thought it was more clear to be direct and use volatile. I understand that not everyone has the same understanding though so I can also see your point about the macro. It does spell out in bold letters what is being done. |
|
Thanks for the link. I'm gonna read it later, but you're comment already makes some stuff better to understand. |
|
Have you been able to test it yet? That was the hard part I think. |
|
Nope, no chance reproducing it so far, so basically this will be a shot in the dark anyway |
|
@findchris Did you already try running hiredis-rb off this PR? Would be nice to know if at least it seems to fix your issue. |
|
There |
|
There might be a couple more places where the compiler could use a register instead but without a way to test it is tough to know. Interesting the note about buggy compilers. Something to keep in mind as a possibility. Anyway, if you want to drop this PR thats okay by me. I mostly trying to help out and you seem to have it under control. |
|
@ohler55 I'm very thankful for the PR and your help here, I'm still undecided what way to go. |
|
I can try running this PR against production, but it doesn't appear that it would contain the |
|
Well, mixing them wouldn't give us a better idea whether one or the other solution works, so it would be nice if you try the |
|
@findchris Any word from production yet? |
|
@findchris Thanks, I'm pulling this in now. |
|
@not-A-robot r+ |
|
📌 Commit 0fb7284 has been approved by |
Foiling CG Made most VALUE variables volatile to stop them from being GCed until they are off the stack. I probably changed a couple that did not need to be changed but making the variable volatile will not hurt anything. What it does is stop the compiler from optimizing those variable into registers when Ruby does not see them. They remain on the stack and Ruby will not CG them until they are no longer on the stack. I also left a FIXME comment related to the redisreaderi->reply use. It might be worth doing a double check.
|
☀️ Test successful - status-travis |
ghost
merged commit 
Made most VALUE variables volatile to stop them from being GCed until they are off the stack.
I probably changed a couple that did not need to be changed but making the variable volatile will not hurt anything. What it does is stop the compiler from optimizing those variable into registers when Ruby does not see them. They remain on the stack and Ruby will not CG them until they are no longer on the stack.
I also left a FIXME comment related to the redisreaderi->reply use. It might be worth doing a double check.