Dynamically detect recursion. #122
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
vext01
reviewed
May 2, 2025
| TValue *k; /* constants used by the function */ | ||
| Instruction *code; /* opcodes */ | ||
| #ifdef USE_YK | ||
| /* Used to detect recursive function calls. When a function is |
Contributor
There was a problem hiding this comment.
If I were to nitpick (moi?), I'd format the comment like the others (with a star on each line).
Contributor
|
Very neat :) Just an nitpick comment. |
ltratt
force-pushed
the
only_trace_recursive_functions
branch
from
e251aa6 to
81241fd
Compare
Contributor
Author
|
Forced push a comment fix. |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Previously we added a `YkLocation` to the start of every Lua function, which meant that we created lots of (mostly) pointless traces. This commit changes things so that only when we dynamically detect that a Lua function is recursive do we add a `YkLocation` to it. This largely prevents us tracing pointless functions. This fix is done in constant time using a trick that took me a long while to realise: we only need to detect that we're in one level of recursion and we only need to do so the first time that happens. A single boolean suffices for this (previously I'd tried being much more precise, but (a) that required examining every call on the stack (b) it turns out not to be necessary): it's always accurate for the first recursive call, and only becomes inaccurate after that point (at which point we don't need it anyway).
ltratt
force-pushed
the
only_trace_recursive_functions
branch
from
81241fd to
a9bc71b
Compare
Contributor
Author
|
Force pushed an |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously we added a
YkLocationto the start of every Lua function, which meant that we created lots of (mostly) pointless traces.This commit changes things so that only when we dynamically detect that a Lua function is recursive do we add a
YkLocationto it. This largely prevents us tracing pointless functions.This fix is done in constant time using a trick that took me a long while to realise: we only need to detect that we're in one level of recursion and we only need to do so the first time that happens. A single boolean suffices for this (previously I'd tried being much more precise, but (a) that required examining every call on the stack (b) it turns out not to be necessary): it's always accurate for the first recursive call, and only becomes inaccurate after that point (at which point we don't need it anyway).