Skip to content

Conversation

unlessgames
Copy link
Collaborator

Chained expressions were generating unexpectedly thrown away results thanks to the way they were parsed, fixing that here.

Also did a bit of cleanup by separated the Target, Bjorklund and Degrade operators from Dynamic/Dynamic Op/Expression, as they were treated special anyway, this makes the types better reflect the behaviour and leads to a bit less branching at the cost of a few more types.

A few more test cases would be nice to add before merge.

Copy link

Benchmark for 73cd84b

Click to view benchmark
Test Base PR %
Cycle/Generate 49.2±0.40µs 52.6±0.57µs +6.91%
Cycle/Parse 366.2±5.92µs 350.8±7.16µs -4.21%
Rust Phrase/Clone 443.9±22.47ns 445.8±7.27ns +0.43%
Rust Phrase/Create 71.4±1.40µs 71.8±1.39µs +0.56%
Rust Phrase/Run 642.7±8.15µs 657.2±5.69µs +2.26%
Rust Phrase/Seek 150.6±281.26µs 151.8±283.19µs +0.80%
Scripted Phrase/Clone 619.2±9.10ns 623.9±8.08ns +0.76%
Scripted Phrase/Create 956.9±12.92µs 959.5±8.35µs +0.27%
Scripted Phrase/Run 1666.1±7.94µs 1676.1±13.96µs +0.60%
Scripted Phrase/Seek 216.0±436.10µs 213.3±429.60µs -1.25%

Copy link

Benchmark for d82248e

Click to view benchmark
Test Base PR %
Cycle/Generate 48.9±0.35µs 49.4±0.36µs +1.02%
Cycle/Parse 362.8±4.75µs 354.2±6.89µs -2.37%
Rust Phrase/Clone 434.6±9.76ns 443.3±5.24ns +2.00%
Rust Phrase/Create 71.6±1.24µs 73.2±2.09µs +2.23%
Rust Phrase/Run 632.6±6.69µs 631.8±5.04µs -0.13%
Rust Phrase/Seek 146.2±271.87µs 146.9±272.84µs +0.48%
Scripted Phrase/Clone 626.4±8.20ns 618.8±5.06ns -1.21%
Scripted Phrase/Create 950.7±8.14µs 959.4±11.43µs +0.92%
Scripted Phrase/Run 1649.0±9.96µs 1628.5±21.21µs -1.24%
Scripted Phrase/Seek 221.1±447.13µs 211.6±426.74µs -4.30%

Copy link

Benchmark for bcfdb47

Click to view benchmark
Test Base PR %
Cycle/Generate 48.7±0.51µs 48.7±0.46µs 0.00%
Cycle/Parse 363.4±5.88µs 363.9±5.56µs +0.14%
Rust Phrase/Clone 430.4±5.07ns 434.0±4.74ns +0.84%
Rust Phrase/Create 71.7±1.17µs 73.9±2.68µs +3.07%
Rust Phrase/Run 641.7±5.36µs 647.6±3.97µs +0.92%
Rust Phrase/Seek 144.1±268.90µs 141.9±265.01µs -1.53%
Scripted Phrase/Clone 613.0±11.00ns 613.1±4.86ns +0.02%
Scripted Phrase/Create 971.2±13.92µs 961.5±12.87µs -1.00%
Scripted Phrase/Run 1617.8±14.39µs 1625.4±6.74µs +0.47%
Scripted Phrase/Seek 217.6±441.03µs 201.9±403.57µs -7.22%

@unlessgames unlessgames marked this pull request as ready for review May 16, 2025 11:27
@unlessgames unlessgames requested a review from emuell May 16, 2025 11:27
@unlessgames
Copy link
Collaborator Author

So this seems ok, added some tests, found and fixed a bug in the meantime.

If it looks good we should merge it as I have other stuff in the works that rely on this PR.

Copy link
Member

@emuell emuell left a comment

Choose a reason for hiding this comment

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

Looks all good to me. Will do more testing after this got merged...

@emuell emuell merged commit 3aa1014 into master May 16, 2025
2 checks passed
@emuell emuell deleted the fix/chained-expressions branch May 16, 2025 12:06
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