Skip to content

Conversation

@miniBill
Copy link
Contributor

@miniBill miniBill commented Dec 7, 2024

image

@jfmengels
Copy link
Collaborator

This is super weird, because I can reproduce tests being many time faster, but when I run the performance script

./find-regressions.sh --no-check ~/.elm/0.19.1/packages/

then performance is about the same, maybe even worse

Published: avg 1.5168321927699087
Current:   avg 1.5336765493284654
Diff: -1.0982991534904896% faster

@miniBill miniBill force-pushed the optimize-char-comparisons branch from 24c3d16 to eb088b1 Compare December 7, 2024 16:16
False

_ ->
True
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe it's time we extract this to a named function 😅

@miniBill miniBill force-pushed the optimize-char-comparisons branch from eb088b1 to 37a337b Compare December 7, 2024 16:34
@lue-bird
Copy link
Contributor

lue-bird commented Dec 7, 2024

Using the code in benchmark/ I also don't notice any improvement.
So I imagine this difference in the test may be caused by our existing code not getting optimized outside of eol2 (== literal vs case).

In any case we should merge this.

@jfmengels
Copy link
Collaborator

Right, I agree I think it's a difference between having --optimize and not having it. And this change manually gets us closer to that. It's impressive that these tiny changes have so much impact on performance in non-optimized though.

Even if the final performance does not get improved (which it might, just a bit hard to measure), it makes tests run much faster, and that's a nice win for the people working on this project!

Thank you @miniBill ❤️

@jfmengels jfmengels merged commit 37a337b into stil4m:master Dec 7, 2024
2 checks passed
@jfmengels
Copy link
Collaborator

@lue-bird do you think this is worth releasing? 🤔

@miniBill miniBill deleted the optimize-char-comparisons branch December 7, 2024 17:00
@jfmengels
Copy link
Collaborator

jfmengels commented Dec 7, 2024

Published in 7.3.8!
This is also going to make elm-review faster when run with --debug, as well as elm-review rule tests ❤️

@lue-bird
Copy link
Contributor

lue-bird commented Dec 7, 2024

I applied the same changes for elm-syntax-format's lenient parser and got a 5-10% improvement (with eol2 applied) and 3.x for non eol2. That's amazing.

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.

3 participants