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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

add lazy_string in matrix2.pyx #35039

Merged
merged 3 commits into from
Apr 13, 2023
Merged

add lazy_string in matrix2.pyx #35039

merged 3 commits into from
Apr 13, 2023

Conversation

xcaruso
Copy link
Contributor

@xcaruso xcaruso commented Feb 9, 2023

馃摎 Description

Formatting verbose messages in the method right_kernel could be quite slow (even when the message is never printed). We use the functionality lazy_string in order to avoid the formatting.

Also, just calling verbose() is slow when just a cache lookup is needed.

Timings

Before:

sage: K.<a> = NumberField(x^2 - 2, embedding=1)
sage: A = MatrixSpace(K, 1, 1).one()
sage: %time _ = A.right_kernel()
CPU times: user 23 ms, sys: 3 ms, total: 26 ms
Wall time: 26.1 ms
sage: %timeit A.right_kernel()  # cache lookup
2.16 碌s 卤 70.3 ns per loop (mean 卤 std. dev. of 7 runs, 100,000 loops each)

After:

sage: K.<a> = NumberField(x^2 - 2, embedding=1)
sage: A = MatrixSpace(K, 1, 1).one()
sage: %time _ = A.right_kernel()
CPU times: user 11 ms, sys: 10 碌s, total: 11 ms
Wall time: 11.2 ms
sage: %timeit A.right_kernel()  # cache lookup
103 ns 卤 1.55 ns per loop (mean 卤 std. dev. of 7 runs, 10,000,000 loops each)

馃摑 Checklist

  • I have made sure that the title is self-explanatory and the description concisely explains the PR.
  • I have linked an issue or discussion. No, this was discussed at SageDays 117, we have no linked issue on GitHub for this.
  • I have created tests covering the changes. No, there are already tests testing that verbose still works correctly.
  • I have updated the documentation accordingly. No, there are no user-facing changes.

@saraedum
Copy link
Member

saraedum commented Feb 9, 2023

@xcaruso it seems that this does not do the trick yet. See timings.

@xcaruso
Copy link
Contributor Author

xcaruso commented Feb 9, 2023

Yes, I saw.

@saraedum
Copy link
Member

saraedum commented Feb 9, 2023

That's a bit better now. Let's ask py-spy what's going on now.

@saraedum
Copy link
Member

saraedum commented Feb 9, 2023

That's funny:

image

@saraedum
Copy link
Member

saraedum commented Feb 9, 2023

The value is taken from the cache now, so all the cost we are paying is the empty call to verbose at the top.

@saraedum
Copy link
Member

saraedum commented Feb 9, 2023

So, that's a bit sad: @roed314 @videlec

sage: %timeit verbose("")
1.58 碌s 卤 10.6 ns per loop (mean 卤 std. dev. of 7 runs, 1,000,000 loops each)

verbose returns cputime so that we can in later calls print the elapsed time鈥e should probably disable this when the verbosity level is not reached. This breaks constructions like this:

tm = verbose("", level=999)
verbose("shows elapsed time", level=1, t=tm)

@saraedum
Copy link
Member

saraedum commented Feb 9, 2023

Should we come up with a plan later today to add regression testing to SageMath so we have some chance to catch things like that in the future? @fchapoton @roed314 @videlec

Copy link
Contributor

@fchapoton fchapoton left a comment

Choose a reason for hiding this comment

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

looks good

@seblabbe seblabbe self-requested a review February 10, 2023 22:21
Copy link
Contributor

@seblabbe seblabbe left a comment

Choose a reason for hiding this comment

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

Looks good to me

@tscrim tscrim requested a review from seblabbe February 11, 2023 01:23
@vbraun vbraun merged commit f407442 into sagemath:develop Apr 13, 2023
1 check passed
@mkoeppe mkoeppe added this to the sage-10.0 milestone Apr 14, 2023
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

7 participants