Skip to content

Conversation

@ltratt
Copy link
Contributor

@ltratt ltratt commented Feb 14, 2025

[Needs https://github.com/ykjit/yk/pull/1606 merged first.]

This commit, small though it is, makes two changes to how we inform yk about Lua loops:

  1. We now calculate where loops start when creating bytecode (rather than continually examining every opcode in the main loop).

  2. We now identify the beginning of the first instruction in a loop as the start of a loop. Previously we identified the beginning of the last instruction in a loop as the start of the loop.

The last change can make ~10% difference on some of the benchmarks I tried, because the trace we get back is both less likely to deopt at the "wrong points" and is more amenable to trace optimisation.

This commit, small though it is, makes two changes to how we inform
yk about Lua loops:

  1. We now calculate where loops start when creating bytecode (rather
     than continually examining every opcode in the main loop).

  2. We now identify the beginning of the first instruction in a loop as
     the start of a loop. Previously we identified the beginning of the
     last instruction in a loop as the start of the loop.

The last change can make ~10% difference on some of the benchmarks I
tried, because the trace we get back is both less likely to deopt at the
"wrong points" and is more amenable to trace optimisation.
@vext01
Copy link
Contributor

vext01 commented Feb 17, 2025

LGTM. Will wait for the dependency PR to merge.

@ltratt
Copy link
Contributor Author

ltratt commented Feb 17, 2025

@vext01 I think we can try this now.

@vext01 vext01 added this pull request to the merge queue Feb 17, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Feb 17, 2025
@ltratt
Copy link
Contributor Author

ltratt commented Feb 17, 2025

We might need to wait for the patching patch to be patched in before this patch will merge.

@ltratt
Copy link
Contributor Author

ltratt commented Feb 17, 2025

Oops, I forgot there's a second version. Forced pushed identical changes to the first. Should hopefully be ready to merge.

@vext01
Copy link
Contributor

vext01 commented Feb 18, 2025

Are you sure you force pushed? I don't see it.

@ltratt
Copy link
Contributor Author

ltratt commented Feb 18, 2025

Oops, wrong PR!

@vext01 vext01 added this pull request to the merge queue Feb 19, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Feb 19, 2025
@ltratt
Copy link
Contributor Author

ltratt commented Feb 19, 2025

We need the release-with-asserts commit for this PR to merge, but there's a circular dependency which means that fix can't merge on its own. I've cherry picked it here and hopefully this "combined" PR should now merge.

@vext01 vext01 added this pull request to the merge queue Feb 20, 2025
Merged via the queue into ykjit:main with commit 8a7a208 Feb 20, 2025
2 checks passed
@ltratt ltratt deleted the rethink_locations branch April 17, 2025 07:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants