Faster time decoder #20
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…tions when using ruby >= 2.3 - SimpleDecoder::TimestampWithTimeZone::decode - SimpleDecoder::TimestampWithoutTimeZone::decode
…when the string can't be parsed.
- Don't fail when the time is more precise than processed - Shorten some expect-series. - Change Spaces->Tabs. - Add a test case for leap-seconds.
…c_new() Both _mkgmtime() as well as mktime() return errors with certain dates. This path is approx. 10 times slower, compared with rb_time_timespec_new(). Its still 10 times faster than the previous RegExp way.
Also fall through to the slow path, because it fails on Android (using termux)
with certain time values otherwise:
Failures:
1) PG::Type derivations PG::SimpleCoder#decode timestamps decodes timestamps with date before 1823
Failure/Error:
expect( textdec_timestamp.decode('1822-01-02 23:23:59.123456') ).
to be_within(0.000001).of( Time.new(1822,01,02, 23, 23, 59.123456) )
expected "1822-01-02 23:23:59.123456" to be within 1.0e-06 of 1822-01-02 23:23:59 +0100, but it could not be treated as a numeric value
# ./spec/pg/type_spec.rb:125:in `block (5 levels) in <top (required)>'
# ./spec/helpers.rb:31:in `block in included'
2) PG::Type derivations PG::SimpleCoder#decode timestamps decodes timestamps with date after 2116
Failure/Error:
expect( textdec_timestamp.decode('2117-01-02 23:23:59.123456') ).
to be_within(0.000001).of( Time.new(2117,01,02, 23, 23, 59.123456) )
expected "2117-01-02 23:23:59.123456" to be within 1.0e-06 of 2117-01-02 23:23:59 +0100, but it could not be treated as a numeric value
# ./spec/pg/type_spec.rb:129:in `block (5 levels) in <top (required)>'
# ./spec/helpers.rb:31:in `block in included'
Finished in 1 minute 19.47 seconds (files took 1.27 seconds to load)
386 examples, 2 failures, 1 pending
ged
approved these changes
Apr 30, 2018
|
@ged Thank you very much for reviewing! I'll merge it when time permits. Moreover I think it makes sense to add another de+encoder for timestamps without timezone, which are treated as UTC time instead of local time. This is how ActiveRecord handles timestamps without timezone per default. |
|
Thank you for merging this! It's a big speed boost for us as we're using the raw exec for a hefty query. Thank you for your cleanup @larskanis |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This moves PG::SimpleDecoder::TimestampWithTimeZone and PG::SimpleDecoder::TimestampWithoutTimeZone to C. It is based on #19 with some tweaking by me.
I tested the implementation successfully on Windows-10 with MINGW (RubyInstaller), Ubuntu-18.04 with glibc, Alpine-Linux-3.7 with musl-libc and Android-7.0 with bionic-libc (termux).
The conversion is done by a fast path and as a fallback by a slower path. The slow path is used under these conditions:
timegm()is not available (on Windows)RUBY_VERSION < 2.3.0(it doesn't providerb_time_timespec_new())timegm()ormktime()fails at runtime (happens on Android)I did some very simple benchmarks with the following results: