Re-enable tab completion of kwargs for large method tables

[mbauman]

while testing to ensure that absurdly large method tables tab completing over an abstract function call doesn't tank the performance of the REPL

Fixes #57836

[vtjnash]

You're just essentially demonstrating here that for your specific test case, the subtyping algorithm always takes the fast path--which is true, but seems hardly relevant to the actual point of having this limit

[mbauman]

I don't understand. These tests are effectively for both this change and #54131. Prior to those patches they fail.

I considered using the show MWE from #57836 but it seemed to be bad mojo to effectively test that the number of show methods was under some limit.

[mbauman]

@vtjnash do you have suggestions on how to improve these tests? Or could you communicate how they're not testing what I think they're testing so I can fix them?

[vtjnash]

You are opting into an algorithm that is too slow for interactive usage (O(n^2)) in measurements when doing anything that doesn't just hit the fast paths like your test, which is why we have these limits. It has nothing to do with display limitations (those limits should not even occur, since display occurs elsewhere). We can opt into a somewhat faster version of the algorithm with -1 (maybe closer to O(n*log n) but nobody has proved that) , but it is also still too slow, delaying keystrokes by much more than 10 ms of added perceptual latency.

This is also just measurements on a simple system image with hardly anything added.

julia> @time Base._methods_by_ftype(Tuple{DataType,Vararg}, 4000, Base.get_world_counter()) |> length
  0.406149 seconds (16.67 k allocations: 1.171 MiB)

julia> @time Base._methods_by_ftype(Tuple{DataType,Vararg}, -1, Base.get_world_counter()) ;
  0.113711 seconds (15.12 k allocations: 1.108 MiB)

julia> @time Base._methods_by_ftype(Tuple{Type,Vararg}, 4000, Base.get_world_counter()) |> length
  0.858004 seconds (59.50 k allocations: 2.917 MiB)
2745

julia> @time Base._methods_by_ftype(Tuple{Type,Vararg}, -1, Base.get_world_counter()) |> length
  0.231704 seconds (50.85 k allocations: 2.568 MiB)
2745

julia> @time Base._methods_by_ftype(Tuple{Type,Vararg}, 40, Base.get_world_counter());
  0.000250 seconds (101 allocations: 7.312 KiB)

julia> @time Base._methods_by_ftype(Tuple{typeof(Core.kwcall),Vararg}, 4000, Base.get_world_counter()) |> length
  0.105716 seconds (8.45 k allocations: 415.672 KiB)
1138

[mbauman]

OK, that's interesting that the limit changes the algorithm — I wasn't aware of that and wouldn't have guessed it. But PR branch is using 500 as its cutoff (not 4000). It's also in contrast with the -1 limit when the REPL has no clue about the function it's calling (the pycall example). I'm trying to restore functionality that the fixes for #54131 broke without impacting perceptual performance in that catastrophic case.

julia> @time Base._methods_by_ftype(Tuple{Any,Vararg}, 40, Base.get_world_counter())
  0.000139 seconds (98 allocations: 7.172 KiB)

julia> @time Base._methods_by_ftype(Tuple{Any,Vararg}, 500, Base.get_world_counter())
  0.005583 seconds (860 allocations: 84.766 KiB)

julia> @time Base._methods_by_ftype(Tuple{Any,Vararg}, -1, Base.get_world_counter())
 12.060503 seconds (863.09 k allocations: 38.049 MiB, 0.04% gc time)

The test-case seems to be adequately representing this distinction. But you may be right that it's still too slow.

julia> function g54131 end
g54131 (generic function with 0 methods)

julia> for i in 1:498
           @eval g54131(::Val{$i}) = i
       end

julia> g54131(::Val{499}; kwarg=true) = 499*kwarg
g54131 (generic function with 499 methods)

julia> @time Base._methods_by_ftype(Tuple{typeof(g54131),Vararg}, 40, Base.get_world_counter())
  0.000058 seconds (49 allocations: 3.078 KiB)

julia> @time Base._methods_by_ftype(Tuple{typeof(g54131),Vararg}, 500, Base.get_world_counter())
  0.017726 seconds (526 allocations: 49.812 KiB)
499-element Vector{Any}:

julia> @time Base._methods_by_ftype(Tuple{typeof(g54131),Vararg}, -1, Base.get_world_counter())
  0.001365 seconds (526 allocations: 49.812 KiB)
499-element Vector{Any}:

Or for show:

julia> @time Base._methods_by_ftype(Tuple{typeof(show),Vararg}, 500, Base.get_world_counter());
  0.002424 seconds (429 allocations: 30.422 KiB)

julia> @time Base._methods_by_ftype(Tuple{typeof(show),Vararg}, -1, Base.get_world_counter());
  0.001357 seconds (410 allocations: 29.656 KiB)

[mbauman]

OK, seems like there could be a better heuristic then. I just pushed a change to disable tab completion in cases where the REPL doesn't know the concrete function — that resolves the PyCall issue (where the function to be called was inferred as Any). This seems like a reasonable restriction; a very large method table may still be a little slow, but it'd take a lot to be catastrophic and it'll be harder to accidentally find yourself in such a situation:

julia> using REPL, REPL.REPLCompletions

julia> @time completions((s="Core.kwcall(lim";), lastindex(s), @__MODULE__, false);
  0.022046 seconds (18.23 k allocations: 934.469 KiB)

julia> function g54131 end
g54131 (generic function with 0 methods)

julia> for i in 1:498
           @eval g54131(::Val{$i}) = i
       end

julia> g54131(::Val{499}; kwarg=true) = 499*kwarg
g54131 (generic function with 499 methods)

julia> @time completions((s="g54131(lim";), lastindex(s), @__MODULE__, false)
  0.002348 seconds (8.19 k allocations: 427.562 KiB)
(REPL.REPLCompletions.Completion[], 8:10, false)

julia> @time completions((s="g54131(kw";), lastindex(s), @__MODULE__, false)
  0.002278 seconds (8.19 k allocations: 427.375 KiB)
(REPL.REPLCompletions.Completion[REPL.REPLCompletions.KeywordArgumentCompletion("kwarg")], 8:9, true)

julia> for i in 500:1000
           @eval g54131(::Val{$i}) = i
       end

julia> @time completions((s="g54131(kw";), lastindex(s), @__MODULE__, false)
  0.006390 seconds (12.21 k allocations: 642.750 KiB)
(REPL.REPLCompletions.Completion[REPL.REPLCompletions.KeywordArgumentCompletion("kwarg")], 8:9, true)

julia> for i in 1001:2000
           @eval g54131(::Val{$i}) = i
       end

julia> @time completions((s="g54131(kw";), lastindex(s), @__MODULE__, false)
  0.020564 seconds (20.22 k allocations: 1.051 MiB)
(REPL.REPLCompletions.Completion[REPL.REPLCompletions.KeywordArgumentCompletion("kwarg")], 8:9, true)

julia> for i in 2001:4000
           @eval g54131(::Val{$i}) = i
       end

julia> @time completions((s="g54131(kw";), lastindex(s), @__MODULE__, false)
  0.073561 seconds (36.22 k allocations: 1.801 MiB)
(REPL.REPLCompletions.Completion[REPL.REPLCompletions.KeywordArgumentCompletion("kwarg")], 8:9, true)

[aviatesk]

I think the implementation change itself is fine.
Regarding tests using @elasped, I think we can just omit them since ultimately what should be tested is our perceived speed, and I feel it is difficult to test this systematically.

[mbauman]

I mean, the reason I added the test was because of #54131 — that's not just being perceptively slow, it's a being so slow you think it crashed. I suppose I could axe the tests, but they represent a real but less-common use case that can be easy to forget about.

[aviatesk]

I understand.
Let's proceed with the merge as is.
If there are any issues with this test case, we can address them when they arise.
Thank you for pushing forward with this PR.