[Compatibility] Implement Time#deconstruct_keys

[rwstauner]

I also tried it with a big case statement (instead of the hash/send)... it's about 10 lines longer, but they appear to bench about the same. 🤷
If you think that (or some other design) would be better let me know.

I updated the one spec because this deconstruct_keys handles unknown symbols differently than the one for MatchData.

[andrykonchin]

I also tried it with a big case statement (instead of the hash/send)... it's about 10 lines longer, but they appear to bench about the same. 🤷

It would be interesting to look at the benchmark results.

[eregon]

I also tried it with a big case statement (instead of the hash/send)... it's about 10 lines longer, but they appear to bench about the same. 🤷

The case should be much better.
The main reason is that public_send with 11 different method names will go megamorphic and that should be a lot slower.

(It would also be possible to do a mix between both approaches by having a constant Hash of Symbol to e.g. -> t { t.hour } and then use THAT_HASH[symbol].call. But that would be a megamorphic block call then which is not great either. So the straightforward case should be best)

[rwstauner]

It would be interesting to look at the benchmark results.

require "benchmark/ips"

t = Time.utc(2022, 10, 5, 13, 30)
res = { year: 2022, month: 10, day: 5, yday: 278, wday: 3, hour: 13,
        min: 30, sec: 0, subsec: 0, dst: false, zone: "UTC" }
slice = res.slice(:year, :month)
def assert_equal(x, y)
  raise "#{x}\n#{y}" unless x == y
end

Benchmark.ips do |x|
  x.report("hash") do
    assert_equal(t.deconstruct_keys(nil) , res)
    assert_equal(t.deconstruct_keys([:year, :month]) , slice)
    assert_equal(t.deconstruct_keys([:year, :month, :day, :yday, :wday, :hour, :min, :sec, :subsec, :dst, :zone]) , res)
  end
  x.report("case") do
    assert_equal(t.deconstruct_keys_case(nil) , res)
    assert_equal(t.deconstruct_keys_case([:year, :month]) , slice)
    assert_equal(t.deconstruct_keys_case([:year, :month, :day, :yday, :wday, :hour, :min, :sec, :subsec, :dst, :zone]) , res)
  end
end

# truffleruby 24.1.0-dev-740a6e18*, like ruby 3.2.2, Interpreted JVM [aarch64-darwin]
# Warming up --------------------------------------
#                 case     2.278k i/100ms
#                 hash     2.481k i/100ms
# Calculating -------------------------------------
#                 case     24.173k (± 1.1%) i/s -    123.012k in   5.089611s
#                 hash     25.475k (± 1.0%) i/s -    129.012k in   5.064732s

# truffleruby 24.1.0-dev-740a6e18*, like ruby 3.2.2, Interpreted JVM [aarch64-darwin]
# Warming up --------------------------------------
#                 case     2.436k i/100ms
#                 hash     2.450k i/100ms
# Calculating -------------------------------------
#                 case     24.485k (± 0.7%) i/s -    124.236k in   5.074140s
#                 hash     25.873k (± 1.1%) i/s -    129.850k in   5.019333s

# truffleruby 24.1.0-dev-740a6e18*, like ruby 3.2.2, Interpreted JVM [aarch64-darwin]
# Warming up --------------------------------------
#                 hash     2.519k i/100ms
#                 case     2.409k i/100ms
# Calculating -------------------------------------
#                 hash     25.325k (± 0.8%) i/s -    128.469k in   5.073215s
#                 case     24.145k (± 1.1%) i/s -    122.859k in   5.089007s

[rwstauner]

Thanks, I appreciate the insights!

[eregon]

Interpreted JVM so that's the issue with that benchmark, it's only benchmarking the interpreter which has lots of other overhead.
Could you rerun it on the jvm-ce env?

[rwstauner]

Oops, thanks, I didn't realize I was running the jvm build.
With jvm-ce there is quite a difference, even if I make the methods hash a constant:

truffleruby 24.1.0-dev-740a6e18*, like ruby 3.2.2, GraalVM CE JVM [aarch64-darwin]
Warming up --------------------------------------
                case    31.541k i/100ms
                hash    52.239k i/100ms
Calculating -------------------------------------
                case      1.038M (± 3.2%) i/s -      5.204M in   5.020972s
                hash    584.887k (± 2.1%) i/s -      2.925M in   5.004034s

[eregon]

The ret[:year] = etc could be moved out of the case like ret[key] = case key.
This avoids the duplication and significantly reduces the memory footprint and # of AST nodes for this method.
@andrykonchin Could you do it (since we have little time before the feature freeze)?

[andrykonchin]

I am on it 👍

[rwstauner]

Ah, that's a good catch!