Skip to content
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

Regenerate constitution golden tests #6785

Merged
merged 2 commits into from
Jan 14, 2025
Merged

Conversation

ana-pantilie
Copy link
Contributor

@ana-pantilie ana-pantilie commented Jan 10, 2025

These are the tests which have been broken on master for a while now. They are slightly less efficient now. We need to update them. AFAIK the pattern matching builtin stuff is what changed the performance here. I don't know how we can fix this performance regression, but until we figure out a way (or maybe @effectfully already has something in mind) we can't just leave these tests to silently fail on master.

@ana-pantilie ana-pantilie added the No Changelog Required Add this to skip the Changelog Check label Jan 10, 2025
@ana-pantilie ana-pantilie requested a review from a team January 10, 2025 15:02
@ana-pantilie ana-pantilie self-assigned this Jan 10, 2025
Copy link
Member

@zliu41 zliu41 left a comment

Choose a reason for hiding this comment

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

I think this came up before but could you remind me why we don't run them in CI?

AFAIK the pattern matching builtin stuff is what changed the performance here

This is surprising. Could you create an issue for investigation?

@ana-pantilie
Copy link
Contributor Author

I think this came up before but could you remind me why we don't run them in CI?

They take too long, there should be a nightly job which runs them but I don't know which one it is and why it isn't failing. Or maybe it is failing and nobody is being notified of the failures.

@bezirg
Copy link
Contributor

bezirg commented Jan 13, 2025

I think this came up before but could you remind me why we don't run them in CI?

They take too long, there should be a nightly job which runs them but I don't know which one it is and why it isn't failing. Or maybe it is failing and nobody is being notified of the failures.

Yes they take too long because all tests (unit,random,golden) are bundled together.
I could separate the golden tests in another testsuite and make only that run by the CI.

@zliu41
Copy link
Member

zliu41 commented Jan 13, 2025

Sounds good @bezirg please open an issue

Comment on lines +499 to +506
\(z : r) (f : a -> list a -> r) (xs : list a) ->
chooseList
{a}
{all dead. r}
xs
(/\dead -> z)
(/\dead -> f (headList {a} xs) (tailList {a} xs))
{r})
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the old way of doing pattern matching, so performance changes are arbitrary. It looks like something has become lazier and something has become stricter, so it's just all noise. Instead of investigating this we should move to pattern matching builtins and then look for optimization opportunities in generated code.

@ana-pantilie ana-pantilie enabled auto-merge (squash) January 14, 2025 12:32
@ana-pantilie ana-pantilie merged commit 38bd591 into master Jan 14, 2025
8 checks passed
@ana-pantilie ana-pantilie deleted the ana/fix-constitution-tests branch January 14, 2025 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
No Changelog Required Add this to skip the Changelog Check
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants