Skip to content

Conversation

@d-torrance
Copy link
Member

@d-torrance d-torrance commented Jul 4, 2025

This reverts #3654, which appears to be responsible for a considerable slowdown.

It appears to be responsible for a considerable slowdown
@d-torrance
Copy link
Member Author

@mikestillman - I think this fixed the slowdown. The cmake-macos build finished in 4.5 hours

@d-torrance d-torrance requested a review from mikestillman July 5, 2025 02:54
@mahrud
Copy link
Member

mahrud commented Jul 5, 2025

Did you revert specific commits, or did you manually undo some changes?

@d-torrance
Copy link
Member Author

Specific commits (in particular, the final 3 commits of the original PR that actually dealt with lastError)

@pzinn
Copy link
Contributor

pzinn commented Jul 5, 2025

I don't think this is a good idea. Instead one should focus on the source of the slow down.
It's most likely due to try being overused, cf the wise advice in the documentation:

users are recommended to use this function sparingly, if at all.

For example, I'm removing try in promote in #3903. Unfortunately this is more complicated than I thought due to issue #3910.

@d-torrance
Copy link
Member Author

Good point.

I did a little experimenting and I think the big slowdown is the try in package(Symbol). I'll push an alternate PR and see if refactoring that helps

@d-torrance
Copy link
Member Author

It's more than just package(Symbol)... There are quite a few things that use try and ??= that are slowing things down.

@mahrud
Copy link
Member

mahrud commented Jul 5, 2025

  1. I'm not opposed to solutions that don't use try, for instance in promote, but try is a useful and sometimes irreplaceable feature, and that recommendation you cite is from almost 28 years ago (647f753), so I wouldn't call try "overused".
  2. lastError isn't being used anywhere yet, so reverting this PR until it can be correctly implemented is reasonable. In particular, as I pointed out in my last comment in "try" should suppress error processing #3848 (comment), preparing the error message should not happen until we actually want to print it. I gave an example where producing the error computes a Groebner basis, which definitely should not happen inside try if the error message won't even be printed (a basic principle in functional programming).

@mahrud
Copy link
Member

mahrud commented Jul 5, 2025

For future reference, I think this feature could be re-implemented as a function printLastError() which only upon being called processes the error, along with a variable lastErrorLocation which produces the location of the error.

@pzinn
Copy link
Contributor

pzinn commented Jul 5, 2025

I thought ??= didn't cause updating lastError? if so it's a valid alternative in many circumstances to try.

@d-torrance
Copy link
Member Author

It does. try and ?? both use tryEval under the hood, which evaluates the code and suppresses errors.

@d-torrance d-torrance merged commit 633939f into Macaulay2:development Jul 9, 2025
5 checks passed
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