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

Turn FinGenAbGroupElem into a non-mutable struct #1621

Merged
merged 1 commit into from
Sep 22, 2024

Conversation

fingolfin
Copy link
Contributor

This reduce allocations and increases speed of code using these elements.

Using the same example as in oscar-system/Oscar.jl#4119, we get the following.

Before this PR here:

julia> @btime is_homogeneous(f);
  1.533 μs (62 allocations: 2.28 KiB)

julia> @btime is_homogeneous(F);
  3.932 μs (152 allocations: 5.53 KiB)

With just this PR:

julia> @btime is_homogeneous(f);
  1.404 μs (48 allocations: 2.00 KiB)

julia> @btime is_homogeneous(F);
  3.688 μs (116 allocations: 4.56 KiB)

With just oscar-system/Oscar.jl#4119:

julia> @btime is_homogeneous(f);
  334.071 ns (10 allocations: 416 bytes)

julia> @btime is_homogeneous(F);
  556.150 ns (12 allocations: 656 bytes)

With this PR and oscar-system/Oscar.jl#4119:

julia> @btime is_homogeneous(f);
  330.772 ns (8 allocations: 352 bytes)

julia> @btime is_homogeneous(F);
  564.865 ns (10 allocations: 592 bytes)

So allocations are further reduced but performance stays the same. But that's OK because in that example only relatively few FinGenAbGroupElem are used after oscar-system/Oscar.jl#4119 ; but many other places still use them extensively, so it still seems quite useful to me to have this.

@@ -2241,11 +2246,11 @@ mutable struct qAdicRootCtx
lf = Hecke.factor_mod_pk(Array, H, 1)
if splitting_field
d = lcm([degree(y[1]) for y = lf])
R = qadic_field(p, d, precision = 1)[1]
R = QadicField(p, d, 1)[1]
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this change about? I thought QadicField is deprecated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Huh, that should not be here. Looks like by accident some old file content snuck in. Will fix it ASAP

This reduce allocations and increases speed of code using
these elements.
@thofma
Copy link
Owner

thofma commented Sep 21, 2024

I don't see the harm. I guess many more element objects could/should be immutable.

Copy link

codecov bot commented Sep 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.85%. Comparing base (d9b7787) to head (3bcb770).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1621      +/-   ##
==========================================
+ Coverage   75.78%   75.85%   +0.07%     
==========================================
  Files         361      361              
  Lines      113695   113692       -3     
==========================================
+ Hits        86164    86242      +78     
+ Misses      27531    27450      -81     
Files with missing lines Coverage Δ
src/GrpAb/Elem.jl 96.64% <ø> (-0.11%) ⬇️
src/HeckeTypes.jl 84.40% <100.00%> (+0.03%) ⬆️

... and 34 files with indirect coverage changes

@thofma thofma merged commit 2601ba7 into thofma:master Sep 22, 2024
17 checks passed
@fingolfin fingolfin deleted the mh/immutable-FinGenAbGroupElem branch September 23, 2024 07:54
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