Skip to content

Remove proto_version.#141

Closed
ltratt wants to merge 1 commit intoykjit:mainfrom
ltratt:proto_version
Closed

Remove proto_version.#141
ltratt wants to merge 1 commit intoykjit:mainfrom
ltratt:proto_version

Conversation

@ltratt
Copy link
Contributor

@ltratt ltratt commented Mar 23, 2026

I added this ages back in response to a crash, when I laboured under the incorrect belief that Lua allows opcode sequences to changed. It doesn't. The fact that this once fixed a crash is a pure coincidence!

Notice also that this comment is wrong:

Thus if two Protos are allocated -- at different times! -- at the
same address

yk_idempotent, by definition, is only in the context of a given Location and trace: it can't return different values given this context.

This is a small but measurable speed-up for benchmarks that spend a reasonable amount of time in the interpreter.

I added this ages back in response to a crash, when I laboured under the
incorrect belief that Lua allows opcode sequences to changed. It
doesn't. The fact that this once fixed a crash is a pure coincidence!

Notice also that this comment is wrong:

> Thus if two `Proto`s are allocated -- at different times! -- at the
> same address

`yk_idempotent`, by definition, is only in the context of a given
`Location` and trace: it can't return different values given this
context.

This is a small but measurable speed-up for benchmarks that spend a
reasonable amount of time in the interpreter.
@vext01 vext01 added this pull request to the merge queue Mar 23, 2026
@vext01
Copy link
Contributor

vext01 commented Mar 23, 2026

Deleting code is good!

@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 23, 2026
@ltratt
Copy link
Contributor Author

ltratt commented Mar 23, 2026

Thank goodness for CI! I now see something like this is necessary to ensure Lua-level inlining is stable. I'll close this in favour of a new PR.

@ltratt ltratt closed this Mar 23, 2026
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.

2 participants