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

optimize Products.v #1938

Merged
merged 1 commit into from
Apr 30, 2024
Merged

optimize Products.v #1938

merged 1 commit into from
Apr 30, 2024

Conversation

Alizter
Copy link
Collaborator

@Alizter Alizter commented Apr 30, 2024

This switches apply to nrapply where applicable and a few miscellaneous inlines.

See #1937 for the script that times.

Before

  Time (mean ± σ):      6.421 s ±  0.024 s    [User: 6.202 s, System: 0.195 s]
  Range (min … max):    6.364 s …  6.461 s    10 runs

After

  Time (mean ± σ):      2.588 s ±  0.028 s    [User: 2.402 s, System: 0.176 s]
  Range (min … max):    2.559 s …  2.651 s    10 runs

Signed-off-by: Ali Caglayan <alizter@gmail.com>

<!-- ps-id: 9970e87a-edc2-49b1-9883-fabb57278b1f -->
Copy link
Collaborator

@jdchristensen jdchristensen left a comment

Choose a reason for hiding this comment

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

Nice. I had noticed that this file was the slowest in the library. Even after this change, it's still a bit slow. This file https://jdc.math.uwo.ca/tmp/Products.html shows the slow lines, in case you want to improve things further. But that can also be done in the future.

Comment on lines +106 to +108
nrapply cat_idl; exact _.
- intros a b c f g r i.
apply cat_assoc.
nrapply cat_assoc; exact _.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are these any different from just using rapply?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Seems to be faster as rapply uses typeclasses during unification and nrapply doesn't and trivially solves them later.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When I say seems to be faster, it's the difference between 0.002 and 0.001.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If it's that small, maybe go with the simpler way? But no big deal either way. And it's fine to merge this and deal with the slow Defined and section End lines later.

Comment on lines +903 to +905
3: nrapply cat_assoc; exact _.
2: refine (_ $@L _).
2: apply cat_pr2_fmap01_binprod.
2: nrapply cat_pr2_fmap01_binprod; exact _.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here (and maybe other spots that I missed).

@Alizter
Copy link
Collaborator Author

Alizter commented Apr 30, 2024

@jdchristensen I definitely think there are some better optimizations that could be done. I also inspected the timing htmls and saw that the Defined lines were taking quite a while. I suspect universes may be playing a role with the performance here.

I just wanted to get these easy optimizations out of the way as I add things to this file in #1929.

@Alizter Alizter merged commit 67d7f92 into HoTT:master Apr 30, 2024
23 checks passed
@Alizter Alizter deleted the ps/rr/optimize_products_v branch April 30, 2024 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants