Parse created_at via Time.new instead of Time.iso8601#9583
Conversation
A full ISO-8601 timestamp with seconds (the format compact index v2 sends, e.g. 2026-05-30T08:52:10Z) is recognised by Time.new directly, so we can drop the require "time" and the indirection through Time.iso8601 while keeping the same ArgumentError fallback for malformed input. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Replaces Time.iso8601 (which requires the time library) with Time.new, which natively parses ISO 8601 strings since Ruby 3.2 (the minimum supported version). This avoids the need for require "time" in parse_metadata.
Changes:
- Use
Time.new(value)instead ofTime.iso8601(value)for parsingcreated_at. - Drop the
require "time"sinceTime.newis built-in. - Simplify the begin/rescue to return the value via assignment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| when "created_at" | ||
| value = v.is_a?(Array) ? v.last : v | ||
| if value.is_a?(String) | ||
| require "time" |
There was a problem hiding this comment.
Can't time be required on file load instead?
require in hot loops is always slow, but I suspect Time.iso8601 is faster than Time.new at least on latest Rubies.
Also Time#iso8601 was moved into core, perhaps Time.iso8601 should be as well? https://bugs.ruby-lang.org/issues/20707
There was a problem hiding this comment.
Time.new appears to be an order of magnitude faster than Time.iso8601.
$ ruby -rtime -rbenchmark -e 's = ARGV.shift; n = 10000; Benchmark.bm {|x| x.report("Time.new") {n.times{Time.new(s)}}; x.report("Time.iso8601") {n.times{Time.iso8601(s)}}}' "2026-05-12T10:00:00Z"
user system total real
Time.new 0.001954 0.000122 0.002076 ( 0.002084)
Time.iso8601 0.012735 0.000192 0.012927 ( 0.012939)
There was a problem hiding this comment.
My rough benchmark Time.new is also 10x faster than Time.iso8601 on current Ruby.
ruby -e '
require "benchmark/ips"
require "time"
sample = "2026-05-30T08:52:10Z"
Benchmark.ips do |x|
x.report("Time.new") { Time.new(sample) }
x.report("Time.iso8601") { Time.iso8601(sample) }
x.compare!
end
'
ruby 4.1.0dev (2026-06-02T04:45:55Z master 056ae1f926) +YJIT +MN +PRISM [arm64-darwin25]
Warming up --------------------------------------
Time.new 348.371k i/100ms
Time.iso8601 74.746k i/100ms
Calculating -------------------------------------
Time.new 4.781M (±17.0%) i/s (209.14 ns/i) - 24.038M in 5.027216s
Time.iso8601 489.645k (±17.9%) i/s (2.04 μs/i) - 2.467M in 5.037563s
Comparison:
Time.new: 4781493.2 i/s
Time.iso8601: 489645.1 i/s - 9.77x slower
There was a problem hiding this comment.
That's interesting. You'd think knowing the string is supposed to be in a precise format would allow iso8601 to be much faster, but I suppose it just mean no-one looked at optimizing it yet.
There was a problem hiding this comment.
Oh... it's implemented with a regexp: https://github.com/ruby/ruby/blob/220f66a33eae1370aa964e7c07efdfdaf7bccddf/lib/time.rb#L626-L658
Parse `created_at` via `Time.new` instead of `Time.iso8601` (cherry picked from commit cc2e37c)
From @nobu's suggestion
Make sure the following tasks are checked