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

MA.operate!(+, pol1, pol2): prevent captured variable perf issue #145

Merged
merged 1 commit into from
Nov 30, 2023

Conversation

nsajko
Copy link
Contributor

@nsajko nsajko commented Nov 27, 2023

The local function was spuriously recursive, making it not recursive fixes the performance problem. That is, this removes some run time dispatch and decreases the amount of allocation, but there's still some run time dispatch left in other places, according to JET.jl.

Before:

julia> import MutableArithmetics; const MA = MutableArithmetics;

julia> using DynamicPolynomials

julia> @polyvar x y;

julia> p = 2x + y;

julia> q = x + 2y;

julia> @allocated MA.operate!(+, p, q)
34413936

After:

julia> import MutableArithmetics; const MA = MutableArithmetics;

julia> using DynamicPolynomials

julia> @polyvar x y;

julia> p = 2x + y;

julia> q = x + 2y;

julia> @allocated MA.operate!(+, p, q)
30835632

Both REPL runs was with nightly Julia v1.11.

The local function was spuriously recursive, making it not recursive
fixes the performance problem. That is, this removes some run time
dispatch and decreases the amount of allocation, but there's still
some run time dispatch left in other places, according to JET.jl.

Before:

```julia-repl
julia> import MutableArithmetics; const MA = MutableArithmetics;

julia> using DynamicPolynomials

julia> @PolyVar x y;

julia> p = 2x + y;

julia> q = x + 2y;

julia> @allocated MA.operate!(+, p, q)
34413936
```

After:

```julia-repl
julia> import MutableArithmetics; const MA = MutableArithmetics;

julia> using DynamicPolynomials

julia> @PolyVar x y;

julia> p = 2x + y;

julia> q = x + 2y;

julia> @allocated MA.operate!(+, p, q)
30835632
```

Both REPL runs was with nightly Julia v1.11.
@blegat
Copy link
Member

blegat commented Nov 30, 2023

It seems the issue is that get1 does not infer correctly ? Maybe fixing this underlying issue will save even more. What about annotating it with _NoVarTerm ?

@nsajko
Copy link
Contributor Author

nsajko commented Nov 30, 2023

Not sure why you think that? Have you checked with Cthulhu or something? The issue that I fixed in this PR is a standard performance of captured variables issue, where the closure is itself the captured variable, so making it nonrecursive prevents boxing, as far as I understand.

Copy link
Member

@blegat blegat left a comment

Choose a reason for hiding this comment

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

No I didn't check, it was just my intuition at the time. It makes sense that breaking recursion prevents allocation

@blegat blegat merged commit b8d6527 into JuliaAlgebra:master Nov 30, 2023
4 checks passed
@nsajko nsajko deleted the captured_var branch November 30, 2023 17:20
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