Skip to content

add loop option #349

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

add loop option #349

wants to merge 4 commits into from

Conversation

zx80
Copy link

@zx80 zx80 commented May 29, 2025

A basic attempt at adding a loop option to report execution time average and standard deviation from a non C++ programmer. Example run:

time /usr/local/bin/jsonschema validate -b --loop 1000 ./openapi_31_20250213.schema.json example.json

Output:

took: 1127.994 +- 41.831 us (0.000)

real    0m54,955s
user    0m54,905s
sys     0m0,041s

So the compilation took about 53 seconds (which seems like a lot?), and running 1000 times over the sample (the benchmark/instance OpenAPI example with the missing "openapi" and "info" properties added) took about 1.1 seconds, so one check is really about 1.12 ms.

@jviotti
Copy link
Member

jviotti commented May 29, 2025

Hey @zx80 , thanks for the PR! I'll take a closer look hopefully tomorrow, but to answer your questions in the mean-time:

So the compilation took about 53 seconds (which seems like a lot?)

Yeah, there was a lot of low-hanging fruits for optimising the compiler (vs the evaluator) which we didn't get a chance to get back to. In our main use case, we compile schemas at built time (so we don't care very much how long it takes), but we are definitely looking for contributions to bring that down.

@zx80
Copy link
Author

zx80 commented May 30, 2025

Argh... Sorry, I cannot make the CI happy. It seems that I do not have the same clang-format version, Ubuntu 24.04 comes with clang 18 by default. Otherwise the feature works:

time /usr/local/bin/jsonschema validate -b --loop 100 ./openapi_31_20250213.schema.json example.json

Output (note that measured overhead is not 0.000 anymore after switching to nano precision):

took: 1138.557 +- 19.813 us (0.015)

real    0m53,524s
user    0m53,476s
sys     0m0,041s

@jviotti
Copy link
Member

jviotti commented May 30, 2025

No worries. I can add relevant commits to fix the style, and tests, etc. One thought: if --loop is only applicable when --benchmark is also set, then what about just making --benchmark take a number instead? Might be a little bit more elegant.

@zx80
Copy link
Author

zx80 commented May 30, 2025

One thought: if --loop is only applicable when --benchmark is also set, then what about just making --benchmark take a number instead? Might be a little bit more elegant.

Fine with me, I just wanted to avoid breaking compatibility.

@jviotti
Copy link
Member

jviotti commented May 30, 2025

I think that's fine! At this stage, I rather keep the CLI clean and simple than avoid breaking compatibility

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