-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Check referential equality before comparing the content when checking String equality. #27790
Conversation
@nanosoldier |
Ah the speedup for |
Your benchmark job has completed - no performance regressions were detected. A full report can be found here. cc @ararslan |
Maybe better add that pointer check to the Julia method then? Thanks to inlining and avoiding a few type-related checks, it could be even faster. |
Could do, but that doesn't fully explain the performance improvement. Nanosoldier saw no significant changes. I am guessing because it does mostly substring comparisons. |
There is a checked conversion from |
Since |
For me this looks like a benchmarking artifact.
edit: Also, shouldn't we expect For a more extreme example, consider
edit2: So I'd advocate for
|
Ok, I put in @chethega 's code. As @nalimilan said
|
Can someone kick the CI? |
Do we want to rerun @nanosoldier ? or are we happy to just merge this? |
Bump |
Co-Authored-By: oxinabox <[email protected]>
Co-Authored-By: oxinabox <[email protected]>
Please squash/merge whenever CI passes. |
Hello, let z = Dict{Symbol, Any}(:class=>"Module"), Main855_1218 = z
function Main858_1221(Main856_1219::Dict)
Main857_1220 = (get)(Main856_1219, :class) do
nothing
end
@info :bug Main857_1220 typeof(Main857_1220) Main857_1220 === "Module"
if Main857_1220 === "Module"
1
else
nothing
end
end
function Main858_1221(Main856_1219)
nothing
end
Main854_1217 = Main858_1221(Main855_1218)
if Main854_1217 === nothing
throw("failed")
else
Main854_1217
end
end
┌ Info: bug
│ Main857_1220 = "Module"
│ typeof(Main857_1220) = String
└ Main857_1220 === "Module" = false
ERROR: "failed"
Stacktrace:
[1] top-level scope at REPL[60]:19 This occurred in my packages and I managed to present the bug in codes without introducing my packages. |
Hence now Py2Jl.jl only works in 1.1.0... |
@thautwarm This PR isn't included in 1.1.1 AFAICT, and anyway it doesn't change the behavior of FWIW, your example code also fails on 1.1.0 here (it works on master, though). Also, why not use |
That issue seems unrelated to this PR but is definitely a regression. Please open a separate issue. |
Issue has already been opened here: #32842. |
This should work because of:
deaf668
for
#22193
I think we might be able to just delete the definition of
==
all together as isn't===
the fallback?Edit: No, it will fallback to
==(a::AbstractString, b::AbstractString)
firstMy microbenchmarks show a fairly solid improvement.
From what I understand of deaf668
this is basically just moving the code from julia into C.
but it is code that was already moved into C, it is just the julia code was left behind.
Here are my benchmarks: