Skip to content

Cache results of kempner_with_data and other related computations#258

Open
fingolfin wants to merge 1 commit intomasterfrom
mh/kempner_cache
Open

Cache results of kempner_with_data and other related computations#258
fingolfin wants to merge 1 commit intomasterfrom
mh/kempner_cache

Conversation

@fingolfin
Copy link
Member

@fingolfin fingolfin commented Feb 21, 2025

This has only a tiny impact for our "standard example" from PR #238 but still seems useful. Tests are done against current Oscar.jl master

Before:

julia> @time scalar_test(g)
 18.572885 seconds (233.76 M allocations: 24.483 GiB, 10.40% gc time, 5.98% compilation time)

julia> @time scalar_test(g)
 17.501142 seconds (230.64 M allocations: 24.328 GiB, 10.84% gc time)

After:

julia> @time scalar_test(g)
 18.635003 seconds (233.66 M allocations: 24.477 GiB, 10.71% gc time, 6.01% compilation time)

julia> @time scalar_test(g)
 17.423853 seconds (230.54 M allocations: 24.322 GiB, 10.94% gc time)

So it just saves ~100000 allocations, but that's not nothing.


I think shorter & quicker micro benchmarks are useful for quick turnaround, so here is a new "standard example" we can copy & paste around:

Before:

julia> using GenericCharacterTables, Chairmarks

julia> g=generic_character_table("3D4.0");

julia> @b scalar_product(g[3],g[7])
3.098 ms (75572 allocs: 3.831 MiB)

julia> @b scalar_product(g[20],g[21])
77.750 ms (995574 allocs: 92.981 MiB, 13.43% gc time)

After:

julia> using GenericCharacterTables, Chairmarks

julia> g=generic_character_table("3D4.0");

julia> @b scalar_product(g[3],g[7])
3.035 ms (75572 allocs: 3.831 MiB)

julia> @b scalar_product(g[20],g[21])
77.214 ms (994886 allocs: 92.936 MiB, 13.85% gc time)

So no change in the first scalar product, and 690 fewer in the second (which is 0.07% ... sigh).

So we really need to go back to some real profiling.

@codecov
Copy link

codecov bot commented Feb 21, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.15%. Comparing base (4d562bb) to head (8012233).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #258      +/-   ##
==========================================
+ Coverage   95.10%   95.15%   +0.04%     
==========================================
  Files          14       14              
  Lines         961      970       +9     
==========================================
+ Hits          914      923       +9     
  Misses         47       47              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@fingolfin
Copy link
Member Author

Seems there is a problem with Oscar 1.1. I'll look into it. In the meantime: any thoughts on this PR, @SoongNoonien ?

@fingolfin fingolfin force-pushed the mh/kempner_cache branch 2 times, most recently from 4cb44e4 to fb071f2 Compare February 22, 2025 23:03
@fingolfin
Copy link
Member Author

Made it work with Oscar 1.1 (see new code comment)

@SoongNoonien
Copy link
Member

In the meantime: any thoughts on this PR

I'm not so sure if it is worth it. I think this makes the code less nice.

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.

2 participants