-
Notifications
You must be signed in to change notification settings - Fork 14
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
YJIT: stop compiling new code for old ISEQs in branch_stub_hit
#531
Comments
Opened a PR on |
Another would be to re-use the If a method was called But that also begs the question of whether branch stubs should be called the first time they are hit, or if they should also be subject to It's very common for Ruby method to have "guard clause" and other infrequently called path for edge cases, it may make sense to prefer side-exiting on these rather than to compile them. Yet another possibility would be to use |
Based on what I'm seeing on Core so far, about 4.5% of branch stubs hit come from old ISEQs, with the fairly conservative threshold that I set (20K blocks since ISEQ first compiled). So we might be able to reduce code size by 4-5% if we implemented this measure, which is not negligible. Could amount to something like a 2% total reduction in YJIT overhead, more if we pick a less conservative threshold. If we combine this with a few other tricks, I can believe that we might be able to shave something like 10% the total YJIT overhead between now and the 3.3 release. Next I want to look at how much we could potentially save by not compiling less hot ISEQs. I will implement counters for Core so that we can graph not-so-hot ISEQs compiled. |
Based on the experiment I've been running in production, it seems that cutting back on compiling cold ISEQs can also cut back on the amount of old branch stubs being hit by 50%+, which I think makes sense. Less cold ISEQs compiled means less "cold branches" being compiled as well. Hot ISEQs will presumably (not always, but as a general rule) execute infrequently executed paths faster. So this means the memory savings we can expect from implementing this idea may be on the order of less than 2%. Can be worth keeping in our back pocket, but it's not a high priority. Cutting back on compiling cold ISEQs can have a much much bigger impact on production workloads. |
I think for now it looks like this maybe isn't worth the added complexity, so I will close it for now, but we could always revisit it later. |
One problem that we want to solve is that code size increases slowly but steadily on Core, eventually reaching very high levels.
Part of the problem seems to be that there is a long tail of code that is not so frequently executed, but the application is large enough that we just keep hitting new code, even if said code is rarely executed. We know that some of this code is new ISEQs being executed, which I hope to address with a new threshold heuristics, but presumably, we're also hitting new branch stubs from old ISEQs later in the execution.
Alan has already put measures in place that allow us to safely exit back to the interpreter from
branch_stub_hit
. That should make it possible to essentially refuse to compile new blocks for a given ISEQ if we want to, and it shouldn't be too hard to implement either.The first question would be what is our definition of old? The lobsters benchmark compiles 67K blocks and we have a counter for this. On Core we seem to go up to 600K. One possible way to detect old ISEQs compiling new blocks would be to keep track of how many blocks were compiled either the first time or the last time we hit an ISEQ. Then we can say, if there have been more than some number of blocks compiled (e.g. 20K) since the last branch stub was hit, this is considered an old ISEQ and we refuse to generate any new code. Alternatively, we could base this on code size, or the address of the last compiled block for an ISEQ, if that's easy/fast to get.
The first step, before implementing this, might be to instrument production to determine how frequently compiling new blocks from old ISEQs actually happen. If it almost never happens, then it might not be a useful optimization. I'll open a PR for this later today, I'm thinking of adding two new counters:
branch_stub_hit
andbranch_stub_hit_old
. Ideally we would want to see thebranch_stub_hit_old
counter keep going up over time on Core.One tricky bit here is that benchmarks are not going to be representative at all. Even our largest benchmark, lobsters, compiles all of its code within something like 3 seconds and then basically nothing after. So we'll have to test this in production, which does seem to have much more of a long tail of code that we can benefit from not compiling.
The text was updated successfully, but these errors were encountered: