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

Avoid @generated in our macros #780

Merged
merged 1 commit into from
Feb 7, 2022

Conversation

fingolfin
Copy link
Member

@fingolfin fingolfin commented Feb 7, 2022

Rewrite @wrap, @gapwrap and @gapattribute to not use @generated anymore: we used that as a trick to delay loading of certain objects from GAP to runtime; but this was always brittle and will fail in the future when Julia becomes more effective and aggressive precompilation support.

We replace this with a simpler approach where the GAP objects now really are only loaded at runtime, and store in global const Ref{GapObj} variables. This is in theory slightly less efficient (as now every function call has to check whether the Ref is already assigned), but this is tiny. In practice we still have a noticeable improvement over directly calling the relevant GAP functions. Indeed:

Before this patch:

julia> @btime GAP.Wrappers.IsString(1);
  6.125 ns (0 allocations: 0 bytes)

julia> @btime GAP.Wrappers.String(1);
  154.647 ns (6 allocations: 160 bytes)

After this patch:

julia> @btime GAP.Wrappers.IsString(1);
  9.844 ns (0 allocations: 0 bytes)

julia> @btime GAP.Wrappers.String(1);
  156.197 ns (6 allocations: 160 bytes)

For reference, without the wrappers (same before and after this patch):

julia> @btime GAP.Globals.IsString(1);
  35.624 ns (0 allocations: 0 bytes)

julia> @btime GAP.Globals.String(1);
  182.200 ns (6 allocations: 160 bytes)

This patch is motivated by JuliaLang/julia#36770 and JuliaLang/julia#43990

@codecov
Copy link

codecov bot commented Feb 7, 2022

Codecov Report

Merging #780 (1f05f90) into master (6824be2) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #780   +/-   ##
=======================================
  Coverage   78.18%   78.19%           
=======================================
  Files          46       46           
  Lines        3645     3642    -3     
=======================================
- Hits         2850     2848    -2     
+ Misses        795      794    -1     
Impacted Files Coverage Δ
src/macros.jl 98.46% <100.00%> (+1.40%) ⬆️
src/GAP.jl 82.17% <0.00%> (ø)

@fingolfin fingolfin changed the title WIP: avoid @generated Avoid @generated in our macros Feb 7, 2022
@fingolfin fingolfin force-pushed the mh/no-generated branch 2 times, most recently from c45b27e to 0558bdf Compare February 7, 2022 09:55
Rewrite `@wrap`, `@gapwrap` and `@gapattribute` to not use `@generated`
anymore: we used that as a trick to delay loading of certain objects
from GAP to runtime; but this was always brittle and will fail in the
future when Julia becomes more effective and aggressive precompilation
support.

We replace this with a simpler approach where the GAP objects now really
are only loaded at runtime, and store in global `const Ref{GapObj}`
variables. This is in theory slightly less efficient (as now every
function call has to check whether the `Ref` is already assigned), but
this is tiny. In practice  we still have a noticeable improvement over
directly calling the relevant GAP functions. Indeed:

Before this patch:

    julia> @Btime GAP.Wrappers.IsString(1);
      6.125 ns (0 allocations: 0 bytes)

    julia> @Btime GAP.Wrappers.String(1);
      154.647 ns (6 allocations: 160 bytes)

After this patch:

    julia> @Btime GAP.Wrappers.IsString(1);
      9.844 ns (0 allocations: 0 bytes)

    julia> @Btime GAP.Wrappers.String(1);
      156.197 ns (6 allocations: 160 bytes)

For reference, without the wrappers (same before and after this patch):

    julia> @Btime GAP.Globals.IsString(1);
      35.624 ns (0 allocations: 0 bytes)

    julia> @Btime GAP.Globals.String(1);
      182.200 ns (6 allocations: 160 bytes)

This patch is motivated by JuliaLang/julia#36770
and JuliaLang/julia#43990
@fingolfin fingolfin merged commit 8e53fac into oscar-system:master Feb 7, 2022
@fingolfin fingolfin deleted the mh/no-generated branch February 7, 2022 14:24
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.

1 participant