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

mkpath always returns the original path #54857

Merged
merged 5 commits into from
Jul 15, 2024
Merged

Conversation

ctarn
Copy link
Contributor

@ctarn ctarn commented Jun 20, 2024

fix #54826

related performance test code:

using BenchmarkTools
using Base: IOError

function checkmode(mode::Integer)
    if !(0 <= mode <= 511)
        throw(ArgumentError("Mode must be between 0 and 511 = 0o777"))
    end
    mode
end

function mkpath(path::AbstractString; mode::Integer = 0o777)
    dir = dirname(path)
    # stop recursion for `""`, `"/"`, or existed dir
    (path == dir || isdir(path)) && return path
    mkpath(dir, mode = checkmode(mode))
    try
        # cases like `mkpath("x/")` will cause an error if `isdir(path)` is skipped
        # the error will not be rethrowed, but it may be slower, and thus we avoid it in advance
        isdir(path) || mkdir(path, mode = mode)
    catch err
        # If there is a problem with making the directory, but the directory
        # does in fact exist, then ignore the error. Else re-throw it.
        if !isa(err, IOError) || !isdir(path)
            rethrow()
        end
    end
    return path
end

function mkpath_2(path::AbstractString; mode::Integer = 0o777)
    dir = dirname(path)
    # stop recursion for `""` and `"/"` or existed dir
    (path == dir || isdir(path)) && return path
    mkpath_2(dir, mode = checkmode(mode))
    try
        mkdir(path, mode = mode)
    catch err
        # If there is a problem with making the directory, but the directory
        # does in fact exist, then ignore the error. Else re-throw it.
        if !isa(err, IOError) || !isdir(path)
            rethrow()
        end
    end
    return path
end

versioninfo()
display(@benchmark begin rm("A", recursive=true, force=true); mkpath("A/B/C/D/") end)
display(@benchmark begin rm("A", recursive=true, force=true); mkpath_2("A/B/C/D/") end)

output:

Julia Version 1.10.4
Commit 48d4fd48430 (2024-06-04 10:41 UTC)
Build Info:
  Official https://julialang.org/ release
Platform Info:
  OS: macOS (x86_64-apple-darwin22.4.0)
  CPU: 16 × Intel(R) Core(TM) i9-9900K CPU @ 3.60GHz
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-15.0.7 (ORCJIT, skylake)
Threads: 1 default, 0 interactive, 1 GC (on 16 virtual cores)
Environment:
  JULIA_EDITOR = code
  JULIA_NUM_THREADS = 
BenchmarkTools.Trial: 8683 samples with 1 evaluation.
 Range (min … max):  473.972 μs …  18.867 ms  ┊ GC (min … max): 0.00% … 0.00%
 Time  (median):     519.704 μs               ┊ GC (median):    0.00%
 Time  (mean ± σ):   571.261 μs ± 378.851 μs  ┊ GC (mean ± σ):  0.00% ± 0.00%

   ▂█▇▄▁                                                         
  ▃███████▇▅▄▄▃▃▃▂▂▂▂▂▂▂▂▂▂▂▂▂▂▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁ ▂
  474 μs           Histogram: frequency by time          961 μs <

 Memory estimate: 5.98 KiB, allocs estimate: 65.
BenchmarkTools.Trial: 6531 samples with 1 evaluation.
 Range (min … max):  588.122 μs …  17.449 ms  ┊ GC (min … max): 0.00% … 0.00%
 Time  (median):     660.071 μs               ┊ GC (median):    0.00%
 Time  (mean ± σ):   760.333 μs ± 615.759 μs  ┊ GC (mean ± σ):  0.00% ± 0.00%

  ██▆▄▃▁▁                                                       ▁
  ██████████▆▇█▇▆▆▅▅▅▄▁▄▁▄▄▃▄▃▁▃▃▁▃▁▁▁▄▁▁▁▁▁▁▁▃▃▁▁▁▃▁▁▃▁▁▁▁▅▅▄▇ █
  588 μs        Histogram: log(frequency) by time        4.2 ms <

 Memory estimate: 5.63 KiB, allocs estimate: 72.

base/file.jl Outdated Show resolved Hide resolved
base/file.jl Outdated Show resolved Hide resolved
base/file.jl Outdated Show resolved Hide resolved
base/file.jl Outdated Show resolved Hide resolved
@ctarn
Copy link
Contributor Author

ctarn commented Jun 20, 2024

Thank you very much! @fingolfin
I have appended some comments accordingly. I am ok with the rest of the mentioned changes, but I would like to leave them as is before strong suggestions. :)

@fatteneder fatteneder added the merge me PR is reviewed. Merge when all tests are passing label Jul 15, 2024
@fatteneder
Copy link
Member

Tests had passed on previous commit, and the new failures are unrelated to this PR.
I am going ahead an merge it.

@fatteneder fatteneder merged commit 7676196 into JuliaLang:master Jul 15, 2024
6 of 8 checks passed
@fatteneder fatteneder removed the merge me PR is reviewed. Merge when all tests are passing label Jul 15, 2024
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.

mkpath does not stably return the original path
3 participants