-
Notifications
You must be signed in to change notification settings - Fork 8
Add Preference to disable runtime invalidation #22
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
Conversation
This makes the package (nearly) `--trim` compatible and also provides a nice preference for users who do not wish for HostCPUFeatures.jl to ever cause an "invalidation storm", even if it means running with the wrong CPU info.
| fast_half() = False() | ||
|
|
||
| @noinline function setfeaturefalse(s) | ||
| @inline function setfeaturefalse(s) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why the inlining switch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was to improve inferrability - has_feature(Val(s)) is only inferrable if the literal value of s is available in the function, which is true in all of the callers (the argument is always a literal Symbol)
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #22 +/- ##
==========================================
- Coverage 36.12% 31.45% -4.68%
==========================================
Files 6 6
Lines 191 213 +22
==========================================
- Hits 69 67 -2
- Misses 122 146 +24 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| reset_extra_features!() | ||
| end | ||
| const BASELINE_CPU_NAME = get_cpu_name() | ||
| const allow_eval = @load_preference("allow_runtime_invalidation", false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not to check that --trim is enabled in JLOptions ?
everyone who will use it with trim will have to find out this culprit themself
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main thinking is that this preference is usable by more than just JuliaC.jl, since some users may wish to be opt-in to error / warn on invalidation storms from this package. JuliaC.jl can set this preference automatically, so the end-user experience is the same.
Also technically checking JLOptions at pre-compilation time won't detect --trim properly, but something like JuliaLang/JuliaC.jl#31 would work
|
As a source of the problem I understand the approach, but it actually downgrades package Pragmatically it is "easier" upgrade standard interface of |
That's already true if |
116a624 to
09a8d77
Compare
…time We already support this dynamically, but for some reason it wasn't possible to get this configuration at precompilation time. Also adds a "freeze_cpu_target" preference to enable this behavior by default.
09a8d77 to
bbd20e1
Compare
| if _has_feature(feature) ≠ has | ||
| @debug "Defining $(has ? "presence" : "absense") of feature $feature." | ||
| set_feature(feature, has) | ||
| if allow_eval |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code doesn't execute if there's a preference right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Depends on which preference you mean
Are you saying you don't think this guard is necessary?
|
Looks like the Julia 1.5 CI is failing. Time to bump minimum version to LTS? |
|
Other than that, LGTM. |
|
@tz-lom are you good with this in current state? If so, I'll merge and release. |
@oscardssmith , thanks, it looks ok. |
|
@tz-lom Anything else you need from me on this? |
|
@topolarity , no that is enough, thank you very much for your work |
|
@topolarity hi Cody, I think we did whoopsie here also check open issue #15 , maybe it can be closed? |
|
I'll bump and release. |
|
@topolarity you missed a compat entry on Preferences.jl |
More conservative version of #21 (thanks to @el-oso for the inspiration).
Adds three preferences:
cpu_target: if provided, use this for feature detection instead ofJULIA_CPU_TARGETfreeze_cpu_target: iftrue, "freeze" the features detected based on your precompile-time CPU targetallow_runtime_invalidation: iffalse, warn instead of invalidating when CPU features don't match precompile-timeThis makes the package
--trimcompatible and also provides a nice preference for users who do not wish for HostCPUFeatures.jl to ever cause an "invalidation storm", even if it means running with the wrong CPU info.