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

Optimize Vector{GapObj} and some other similar cases #736

Open
fingolfin opened this issue Oct 16, 2021 · 0 comments
Open

Optimize Vector{GapObj} and some other similar cases #736

fingolfin opened this issue Oct 16, 2021 · 0 comments
Labels
kind: enhancement New feature or request topic: conversion Issue related to conversion

Comments

@fingolfin
Copy link
Member

fingolfin commented Oct 16, 2021

Consider this little benchmark:

julia> using GAP, BenchmarkTools

julia> L = GAP.evalstr("List([3..6],SymmetricGroup)")
GAP: [ Sym( [ 1 .. 3 ] ), Sym( [ 1 .. 4 ] ), Sym( [ 1 .. 5 ] ), Sym( [ 1 .. 6 ] ) ]

julia> @btime typeof(collect(L))
  582.230 ns (13 allocations: 496 bytes)
Vector{Any} (alias for Array{Any, 1})

julia> @btime typeof([x for x in L])
  653.366 ns (6 allocations: 240 bytes)
Vector{GapObj} (alias for Array{GapObj, 1})

julia> @btime typeof(Vector{GapObj}(L))
  2.285 μs (11 allocations: 608 bytes)
Vector{GapObj} (alias for Array{GapObj, 1})

The performance of Vector{GapObj}(L) is disappointing. I think it is because of the IdDict we create and maintain, but for nought, at least in this case. Also note how collect is the fastest, but returns a suboptimal type.

In this particular case, one can do quite a bit better:

julia> function f(x::GapObj) v=Vector{GapObj}(undef,length(x)); for i in 1:length(x) v[i]=L[i] end ; return v end
f (generic function with 1 method)

julia> @btime typeof(f(L))
  261.013 ns (1 allocation: 112 bytes)
Vector{GapObj} (alias for Array{GapObj, 1})

Perhaps we can set up variants of our conversions which perform faster in "such" cases. Where it remains to be settled what exactly "such" cases are.... coming to mind are certainly:

  • T{S} where T is a container like Vector, Set, ... and S either GapObj or GAP.Obj or Int.
  • it could also be done for T{BigInt} or T{fmpz} but then it'd be a tradeoff in performance vs. memory usage, at least in principle. But it might actually better here, as not all "end users" may expect aliasing in these cases, and if someone starts to modify entries in the result in-place, they might be surprised... ?

Even if we can't solve this in general, I think that at least the T{GapObj} case is important enough to deserve special treatment here. I.e. special methods for Vector{GapObj}(x::GapObj) and perhaps also for collect(x::GapObj)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: enhancement New feature or request topic: conversion Issue related to conversion
Projects
None yet
Development

No branches or pull requests

1 participant