Skip to content

Use gettime on Lua. #20

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

Merged
merged 1 commit into from
Apr 25, 2025
Merged

Use gettime on Lua. #20

merged 1 commit into from
Apr 25, 2025

Conversation

ltratt
Copy link
Contributor

@ltratt ltratt commented Apr 25, 2025

harness.lua uses Lua's os.clock which -- though it's not precisely specified AFAICT -- seems to measure 'user+sys' time rather than the expected wall-clock time. This gives misleading measurements if a process is doing anything other than just running the VM (e.g. it ends up measuring compilation in yklua in threads).

This commit introduces a patch to the normal C Lua VM that introduces a function os.gettime which uses the normal gettime clock. Imperfect though that clock may be, it's a lot better than clock. That patch is applied to both "normal" Lua and yklua.

common.sh Outdated
@@ -26,7 +26,9 @@ setup() {

# Build yklua with JIT support.
git clone https://github.com/ykjit/yklua
cd yklua
cd yklua/src
patch < ../../../../patches/clua_gettime
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing I've learned about patch: always specify a patch level.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works in this case. Do you have a suggested change?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just add a -p.

The problem is, if you don't give patch a -p it starts guessing where to apply the diff and if files of the same name exist in different places, it occasionally gets it wrong. It's bitten me a few times.

Although it works ok now, an future "sync with upstream lua" could cause fun!

I didn't have to search for long to find an example in the wild:
conda/conda-build#4241

In this case I'd cd to the top level dir and use -p0.

@vext01
Copy link
Contributor

vext01 commented Apr 25, 2025

LGTM, just one small comment.

@ltratt
Copy link
Contributor Author

ltratt commented Apr 25, 2025

Point taken. Try that force push.

@ltratt
Copy link
Contributor Author

ltratt commented Apr 25, 2025

Welp, I needed -p0. Fixed (and lightly tested).

@vext01 vext01 added this pull request to the merge queue Apr 25, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 25, 2025
@ltratt
Copy link
Contributor Author

ltratt commented Apr 25, 2025

It turns out that common.sh is called from different directory structures in "normal" mode vs. .buildbot.sh. Fixed in the force push.

@vext01 vext01 added this pull request to the merge queue Apr 25, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 25, 2025
harness.lua uses Lua's `os.clock` which -- though it's not precisely
specified AFAICT -- seems to measure 'user+sys' time rather than the
expected wall-clock time. This gives misleading measurements if a
process is doing anything other than just running the VM (e.g. it ends
up measuring compilation in yklua in threads).

This commit introduces a patch to the normal C Lua VM that introduces a
function `os.gettime` which uses the normal `gettime` clock. Imperfect
though that clock may be, it's a lot better than `clock`. That patch is
applied to both "normal" Lua and yklua.
@ltratt
Copy link
Contributor Author

ltratt commented Apr 25, 2025

I got another path wrong. Fixed in the force push.

@vext01 vext01 added this pull request to the merge queue Apr 25, 2025
Merged via the queue into ykjit:main with commit c177e03 Apr 25, 2025
2 checks passed
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