-
-
Notifications
You must be signed in to change notification settings - Fork 261
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
make tracking for better error details optional (fixes #1009) #1013
Conversation
Warning Rate Limit Exceeded@tomtau has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 21 minutes and 6 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe changes across various files focus on introducing the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
@EmirVildanov FYI this disables tracking for detailed errors to reduce the performance overhead, but it can still be optionally enabled via |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks good. My only thought is: should these checks be in the impacting functions instead of at the call-sites? Seeing if new_state.parse_attempts.enabled { try_add_rule_to_stack(&mut new_state); }
multiple times (along with the others) gives me pause.
I don't know the performance implications of unnecessary function calls, so I'll trust your judgement here, given that we're trying to fix a performance issue. This isn't such a big deal that we should worsen performance more for a little more clarity.
@NoahTheDuke yeah, I was thinking that; I went for checking at call-sites, because 1) it was easier to cross-check against the original PR's modifications in the parser state, 2) some of those calls had a String allocation before that guarded call (maybe the compiler could inline functions there and optimize away those allocations, maybe not). |
With a release of 2.7.10, can 2.7.9 be yanked? Regression was quite big to actually prevent users to use it. |
@klensy that makes sense, so 2.7.9 should be yanked now |
[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [lalrpop](https://togithub.com/lalrpop/lalrpop) | build-dependencies | patch | `0.20.0` -> `0.20.2` | | [lalrpop-util](https://togithub.com/lalrpop/lalrpop) | dependencies | patch | `0.20.0` -> `0.20.2` | | [pest](https://pest.rs/) ([source](https://togithub.com/pest-parser/pest)) | dependencies | patch | `2.7.9` -> `2.7.10` | | [serde_json](https://togithub.com/serde-rs/json) | dependencies | patch | `1.0.116` -> `1.0.117` | | [winnow](https://togithub.com/winnow-rs/winnow) | dependencies | patch | `0.6.7` -> `0.6.9` | --- ### Release Notes <details> <summary>lalrpop/lalrpop (lalrpop)</summary> ### [`v0.20.2`](https://togithub.com/lalrpop/lalrpop/blob/HEAD/RELEASES.md#0202-2024-02-) [Compare Source](https://togithub.com/lalrpop/lalrpop/compare/0.20.1...0.20.2) Special thanks to our newest maintainers, Daniel Burgener and Patrick LaFontaine for helping to coordinate this release. ##### Features - Lalrpop no longer depends on the `is-terminal` crate (thanks to Kmeakin!) - Better performance with the default lexer using the underlying `regex-automata` crate (thanks to QuarticCat!) - Allow the catch-all `_` case for token matching can now be set to a higher precedence in match (thanks to fpoli!) - Fewer clippy lints triggered in generated code - Lalrpop now traverses symlinks to find .lalrpop files(thanks mbid!) - Lalrpop now supports block comments including nestings(thanks seanbright!) ##### Bugfixes - Lalrpop now uses the ascii-aware space regex when the unicode feature is not enabled (thanks to QuarticCat!) - Dangling symlinks in crate no longer cause build failure (thanks to legeana for the report!) - Unicode is now set as a default feature in lalrpop-util to align with lalrpop's defaults ##### Compatibility note - MSRV increased to `1.70`. - `process_root_unconditionally` now correctly lints as having been deprecated. - Internal types which lead with a `__` and should not be relied upon are no longer publicly exposed (thanks to arnaudgolfouse!) - Lalrpop files containing a space in their name now return an error. ### [`v0.20.1`](https://togithub.com/lalrpop/lalrpop/blob/HEAD/RELEASES.md#0201-2024-02-) [Compare Source](https://togithub.com/lalrpop/lalrpop/compare/0.20.0...0.20.1) Yanked </details> <details> <summary>pest-parser/pest (pest)</summary> ### [`v2.7.10`](https://togithub.com/pest-parser/pest/releases/tag/v2.7.10) [Compare Source](https://togithub.com/pest-parser/pest/compare/v2.7.9...v2.7.10) #### What's Changed - make tracking for better error details optional (fixes [#​1009](https://togithub.com/pest-parser/pest/issues/1009)) by [@​tomtau](https://togithub.com/tomtau) in [https://github.com/pest-parser/pest/pull/1013](https://togithub.com/pest-parser/pest/pull/1013) There was a performance regression in 2.7.9 due to the overhead from tracking for better error details. This is now disabled by default, but if you wish to use it, you can enable it via the `pest::set_error_detail(true)` call (before your parsing code starts). **Full Changelog**: pest-parser/pest@v2.7.9...v2.7.10 #### Warning: Semantic Versioning Note that the node tag feature in 2.6.0 was a technically semver-breaking change even though it is a backwards-compatible / non-breaking change in the meta-grammar. There may be similar non-breaking changes to the meta-grammar between minor versions in the future. These non-breaking changes, however, may translate into semver-breaking changes due to the additional variants propagated from the generated `Rule` enum. This new feature caused issues in some Cargo version resolution situations where Cargo mixed different versions of pest dependencies. For this reason, these "grammar non-breaking but semver-breaking" changes are now available only under the "grammar-extras" feature flag. If you would like to use node tags (or other future grammar features), you can do so by enabling this flag on the pest_derive crate in your Cargo.toml: ... pest_derive = { version = "2.7", features = ["grammar-extras"] } </details> <details> <summary>serde-rs/json (serde_json)</summary> ### [`v1.0.117`](https://togithub.com/serde-rs/json/releases/tag/v1.0.117) [Compare Source](https://togithub.com/serde-rs/json/compare/v1.0.116...v1.0.117) - Resolve unexpected_cfgs warning ([#​1130](https://togithub.com/serde-rs/json/issues/1130)) </details> <details> <summary>winnow-rs/winnow (winnow)</summary> ### [`v0.6.9`](https://togithub.com/winnow-rs/winnow/blob/HEAD/CHANGELOG.md#069---2024-05-28) [Compare Source](https://togithub.com/winnow-rs/winnow/compare/v0.6.8...v0.6.9) ##### Compatibility - Bump MSRV to 1.65 ##### Features - Add `Debug` impls for `stream::Stateful` and `stream::Recoverable` ### [`v0.6.8`](https://togithub.com/winnow-rs/winnow/blob/HEAD/CHANGELOG.md#068---2024-05-06) [Compare Source](https://togithub.com/winnow-rs/winnow/compare/v0.6.7...v0.6.8) ##### Features - Support `&mut [impl Parser]` within `alt` </details> --- ### Configuration 📅 **Schedule**: Branch creation - "before 5am on the first day of the month" (UTC), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 👻 **Immortal**: This PR will be recreated if closed unmerged. Get [config help](https://togithub.com/renovatebot/renovate/discussions) if that's undesired. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/rosetta-rs/parse-rosetta-rs). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4zNzcuOCIsInVwZGF0ZWRJblZlciI6IjM3LjM3Ny44IiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6W119--> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
2.7.8 cargo bench:
2.7.9 cargo bench (baseline 2.7.8):
this PR fix when disabled (baseline 2.7.9):