Skip to content

improve regex validation performance.#825

Merged
ahl merged 3 commits into
oxidecomputer:mainfrom
jct-tympanon:static-regex-pr
May 17, 2025
Merged

improve regex validation performance.#825
ahl merged 3 commits into
oxidecomputer:mainfrom
jct-tympanon:static-regex-pr

Conversation

@jct-tympanon
Copy link
Copy Markdown
Contributor

the regexes are cached in a lazy static variable to avoid reparse on each evaluation. parse is expensive compared to eval. added a benchmark suite to verify the change. local results:

before:

long regex              time:   [57.194 µs 57.266 µs 57.372 µs]
short regex             time:   [5.0102 µs 5.0126 µs 5.0150 µs]

after:

long regex              time:   [297.06 ns 297.50 ns 297.96 ns]
short regex             time:   [114.12 ns 114.41 ns 114.72 ns]

the toolchain bump i had to do for local development. I could not bump the toolchain all the way to latest because increased strictness of irrefutable-let warnings regresses several trybuild test cases.

see #682

the regexes are cached in a lazy static variable to avoid reparse
on each evaluation. parse is expensive compared to eval.
added a benchmark suite to verify the change. local results:

before:
    ```
    long regex              time:   [57.194 µs 57.266 µs 57.372 µs]
    short regex             time:   [5.0102 µs 5.0126 µs 5.0150 µs]
    ```

after:
    ```
    long regex              time:   [297.06 ns 297.50 ns 297.96 ns]
    short regex             time:   [114.12 ns 114.41 ns 114.72 ns]
    ```

the toolchain bump i had to do for local development. I could not
bump the toolchain all the way to latest because increased strictness
of irrefutable-let warnings regresses several trybuild test cases.

see oxidecomputer#682
Copy link
Copy Markdown
Collaborator

@ahl ahl left a comment

Choose a reason for hiding this comment

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

Can you tell me about the utility you see of checking in typify-bench? It's useful to see in this case (thank you!), but would you anticipate this being useful to continue using in some capacity? If so how would you imagine its use?

Thanks for submitting this PR; looks close.

Comment thread typify-impl/src/type_entry.rs Outdated
@jct-tympanon
Copy link
Copy Markdown
Contributor Author

jct-tympanon commented May 15, 2025

Can you tell me about the utility you see of checking in typify-bench? It's useful to see in this case (thank you!), but would you anticipate this being useful to continue using in some capacity? If so how would you imagine its use?

Thanks for submitting this PR; looks close.

in this case, IMO when someone commits a "performance improvement", having both the numbers observed and the test case that was exercised next to the commit are good for review, like with unit test cases. for example, if regress develops a macro that emits compile-time regexes, you can swap it in and see the result. for my own practice, i always formalize performance work like this when i'm doing it. it's just nice to have the ground broken so you can throw a test case on the pile the next time you think something might be slow. but, i'm not offended if you'd rather not maintain it, and i can prune it.

the other case is if you want to set performance SLAs or guard against performance regressions; typically you'd set up benchmarks in your CI system and keep the data there and actually fail the build past a certain tolerance. obviously that would require more work. i've done that before, though less frequently (actually doing that with my current project consuming typify).

lastly -- since performance improvements are highly platform-specific, you might want to pull the change down and verify the results locally as part of the review. i'm working on an Apple M1 Max. YMMV (by a lot).

Co-authored-by: Adam Leventhal <adam.leventhal@gmail.com>
@ahl
Copy link
Copy Markdown
Collaborator

ahl commented May 16, 2025

I appreciate the explanation. If you would please prune the test; I don't think we'd maintain it. And we'll have this PR as a record for the future if we ever want to resurrect it. Thank you!

@ahl ahl merged commit 1710967 into oxidecomputer:main May 17, 2025
4 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