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.

Sign up for GitHub

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

Jump to bottom

Add tests for invalidation fixes #47022

Open
rikhuijzer opened this issue Oct 3, 2022 · 1 comment · May be fixed by #47093
Open

Add tests for invalidation fixes #47022

rikhuijzer opened this issue Oct 3, 2022 · 1 comment · May be fixed by #47093
Labels
compiler:latency Compiler latency test This change adds or pertains to unit tests

Comments

@rikhuijzer
Copy link
Contributor

rikhuijzer commented Oct 3, 2022 •
edited
Loading

Invalidation fixes to Julia base are currently not tested. For example:

This is asking for double work in the long run. Would it be possible to add something like SnoopCompileCore.@snoopr to Julia base? This would make it easier to add a tests. For example, #46481 fixes invalidations caused by inserting:

Base.:(!)(::True) = False()

With @snoopr defined as

https://github.com/timholy/SnoopCompile.jl/blob/f4d9a7b57a82c1bdb34579f255d3cf4abcc10b92/SnoopCompileCore/src/snoopr.jl#L26-L35

the method insertion could be tested with:

@test isempty(@snoopr Base.:(!)(::True) = False())

cc: @timholy, @ranocha
@ranocha
Copy link
Member

ranocha commented Oct 3, 2022

Something like this sounds reasonable to me. However, we need to be aware that something like this will still trigger some invalidations, so we could only check some upper bound (that may be increased if required).

rikhuijzer added a commit to rikhuijzer/julia that referenced this issue Oct 4, 2022
@rikhuijzer
Fix show_unquoted invalidations
6b3fc42 
This PR fixes about 1000 method invalidations for multiple separate
packages. The following table lists the number of invalidations on
Julia 1.8.2, Julia nightly and this PR:

Name | 1.8.2 | 1.9.0-DEV.1506 | This PR
--- | --- | --- | ---
CSV | 3173 | 5452 | 4560
CategoricalArrays | 3482 | 4713 | 3668
InlineStrings | 1376 | 654 | 654
ProgressLogging | 246 | 2990 | 2049

Generated with:

```julia
using Pkg

Pkg.activate(; temp=true)

Pkg.add(["SnoopCompileCore", "SnoopCompile", "InlineStrings"])

using SnoopCompileCore

invs = @Snoopr using InlineStrings

using SnoopCompile

trees = invalidation_trees(invs)

print('\n', trees, '\n')

@show length(invs)
```

and running the various Julia versions with `--startup-file=no`.

Note that this PR reverts a change introduced by
JuliaLang#44500.
It changes a `convert(String, X)` back to `string(X)`.

Based on this PR reverting an earlier PR and the seemingly random
increases and decreases in the table, adding invalidations tests should
help avoid regressions
(JuliaLang#47022). I'll see whether I
can find time to work on that.
@rikhuijzer rikhuijzer mentioned this issue Oct 4, 2022
Closed
@ranocha ranocha added compiler:latency Compiler latency test This change adds or pertains to unit tests labels Oct 5, 2022
@rikhuijzer rikhuijzer linked a pull request Oct 7, 2022 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
compiler:latency Compiler latency test This change adds or pertains to unit tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add tests for method invalidation fixes rikhuijzer/julia
2 participants
@ranocha @rikhuijzer