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

Date library using Joda-Time (v2) #890

Merged
merged 100 commits into from Sep 22, 2013
Merged

Date library using Joda-Time (v2) #890

merged 100 commits into from Sep 22, 2013

Conversation

@eregon
Copy link
Member

@eregon eregon commented Jul 16, 2013

The branch was moved to my own fork so I can happily push --force if I need to rework the first commits.

  • Date/DateTime constructors are very liberal for the arguments, they accept negative args and such. Replacing them entirely with Joda is hard and joda-time only handles civil format (y,m,d, h,m,s) at creation. The civil constructor has a fast path for positive args.
  • ajd is computed lazily and used only in a couple places: constructors not supported by joda-time, #ajd and #amjd, #<=>(Integer) and marshaling (since we want MRI compat).
  • Arbitrary precision is kept, with a @sub_millis field, optimized as 0 when not used otherwise as a Rational.
  • {Time,Date}#strftime has been improved a lot, with a proper lexer and better support.
TODO

Improve perf of:

  • strptime
  • other parsing methods
@BanzaiMan
Copy link
Member

@BanzaiMan BanzaiMan commented Jul 18, 2013

Could you rebase and try again? There were problems with the master's build.

@eregon
Copy link
Member Author

@eregon eregon commented Jul 18, 2013

Rebased, this is a work in progress though, I will merge it when it will be much more complete.
The PR is mainly for discussions and review of the code, sorry if I was not clear.
(There should be the mentioned above failure in the build)

eregon added 27 commits Jul 13, 2013
* Tests are unaffected by this change.
* Zone offset and Gregorian cutover are computed from the DateTime.
* They are still copied in instances variables for ease of compatibility with current code.
* @ajd need to be preserved for now as DateTime only has millisecond precision,
  date.rb has arbitrary (although mentioned not expected > ns) (via Rational).
* Joda methods are used for year,week,day,hour,min and second
  (but not fractions since it might be imprecise).
…d on Chronology

* In ISOChronology, it is fine, but in GJChronology, it's century + 1.
* Still a problem for the year right now with GJChronology (but no test!)
… such "times"

* that is earlier than 292 275 054 BC or later than 292 278 993 AD ...
* I so wish millis of DateTime was always 0 and nsec be nanoseconds
…ematic in non-latest joda-time

* also, Ruby handles division of a negative in a way not conforming to this usage
* from logic in date.rb (which is way simpler)
* tests coming in RubySpec
… directly for now

ruby 2.0.0p247
Date.new 924.9 ns/i ± 13.60 ( 1.5%) <=>   1081082 ips

jruby: before
Date.new 20.72 µs/i ± 0.337 ( 1.6%) <=>     48260 ips

jruby: after
Date.new 10.61 µs/i ± 0.102 ( 1.0%) <=>     94207 ips

jruby: without creation of the DateTime
Date.new 7.483 µs/i ± 0.212 ( 2.8%) <=>    133643 ips

jruby: without computation of millis from ajd (through Rationals)
Date.new 4.980 µs/i ± 0.068 ( 1.4%) <=>    200788 ips

* ideal scenario is creating the DateTime from civil parameters directly (year, month, day)
  and never rely on @ajd, but it means rewriting many methods of Date
* Passes test__iso8601

* ruby 2.0.0p247
  Date._iso8601 9.046 µs/i ± 0.053 ( 0.6%) <=>    110548 ips
* before
  Date._iso8601 72.35 µs/i ± 24.68 (34.1%) <=>     13821 ips
* after
  Date._iso8601 5.185 µs/i ± 0.065 ( 1.3%) <=>    192847 ips

* Date.iso8601 is twice faster, bottleneck seems now #complete_frags
…ntend to pass those tests

* Actually the C code simply allocate the Hash upfront and
  pass it to sub-matchers, leaving it untouched if nothing matches.
* but MRI does it too in d_new_by_frags()!
* Takes 21.08µs vs 33.78µs for Date.iso8601("2011-03-08"),
  might not be worth it and get removed.
* single #max_by instead of #map + #select + #sort_by + #last
* simple #count instead of #values_at + #compact
* no use in counting if :hour, :min and :sec match for every format
* do not bother setting those for simple Dates (not DateTimes)
* remove sec = min(sec, 59), this is nonsense!
eregon added 24 commits Sep 2, 2013
before
Date#strftime("%Y-%m-%d") 5.300 µs/i ± 0.020 ( 0.4%) <=>    188668 ips
after
Date#strftime("%Y-%m-%d") 3.624 µs/i ± 0.009 ( 0.3%) <=>    275922 ips
* makes some tests fail unfortunately, MRI accepts offset of +24 or -24
* Tokenize exactly as the old one but in a much more proper way
* Should handle every edge case, with only a grammar of a dozen lines
* Remove massive amount of code from TimeOutputFormatter and RubyDateFormat
* Add a helper for composed formats to enhance readability
* also escape the '+' inside the character class
* the look-ahead part (or a substring) would be captured in the current expression
* JFlex 1.4 seems fine
* need ThreadContext, use constructor to pass it
* pass all related Date/DateTime tests
* more arguments, but casting from Object (long versus Rational) seems bad
* infinite sg is represented specially
* ns rational is simplified if possible
* s is expressed in UTC
ruby 2.0.0p247
dump   8.563 µs/i ± 0.169 ( 2.0%) <=>    116785 ips
load   7.091 µs/i ± 0.446 ( 6.3%) <=>    141015 ips
reload 16.93 µs/i ± 0.346 ( 2.0%) <=>     59059 ips

jruby before with cached ajd
dump   5.991 µs/i ± 0.033 ( 0.6%) <=>    166926 ips
load   15.55 µs/i ± 1.890 (12.1%) <=>     64275 ips
reload 24.53 µs/i ± 0.461 ( 1.9%) <=>     40757 ips

jruby before without cached ajd
dump   8.306 µs/i ± 1.438 (17.3%) <=>    120389 ips
load   15.61 µs/i ± 0.384 ( 2.5%) <=>     64062 ips
reload 27.76 µs/i ± 1.315 ( 4.7%) <=>     36014 ips

jruby using [@dt, @Of, @sg, @sub_millis]
dump   30.56 µs/i ± 0.059 ( 0.2%) <=>     32722 ips
load   123.4 µs/i ± 0.482 ( 0.4%) <=>      8099 ips
reload 165.6 µs/i ± 4.235 ( 2.6%) <=>      6038 ips

jruby using [@dt.getMillis, @Of, @sg, @sub_millis]
dump   5.154 µs/i ± 0.044 ( 0.9%) <=>    194039 ips
load   10.85 µs/i ± 0.062 ( 0.6%) <=>     92154 ips
reload 19.41 µs/i ± 0.082 ( 0.4%) <=>     51505 ips
* ajd is only used in #<=> for comparison with ajd, pretty rare
* #day_fraction is rarely used
* #julian? must be optimized anyway since the off-by-one problem with joda DateTime negative years
* take advantage of the fact the default TZ should not change
* avoid some irrelevant assertions
* using minitest/exclude by method would exclude way too much
eregon added a commit that referenced this pull request Sep 22, 2013
Date library using Joda-Time with arbitrary precision and improved performance
@eregon eregon merged commit f831eaa into jruby:master Sep 22, 2013
1 check was pending
1 check was pending
@veganstraightedge
default The Travis CI build is in progress
Details
@eregon
Copy link
Member Author

@eregon eregon commented Sep 22, 2013

@headius Merged!

@atambo
Copy link
Member

@atambo atambo commented Sep 22, 2013

I think your modifications to the tests in the /test/externals/ will get blown away the next time they are sync'd with mri. Maybe you should just extract those tests out into the /spec/regression/ directory?

Also, I wonder if these patches fix some of these rails/jruby issues about Time marshalling:
rails/rails#10900
rails/rails#11911
#957

@eregon
Copy link
Member Author

@eregon eregon commented Sep 22, 2013

I think your modifications to the tests in the /test/externals/ will get blown away the next time they are sync'd with mri. Maybe you should just extract those tests out into the /spec/regression/ directory?

@atambo Yes, I fear they will indeed get away. There is only one minor additional test. I used that technique to comment uninteresting failing assertions, yet testing other assertions within the same method (since minitest/exclude can only exclude by method I think). Excluding blindly these methods would reduce the coverage too much to my taste. I guess we could copy the passing/interesting assertions to some other suite, but it seems a wrong solution.

What if I merged the test changes in jruby/ruby ?

I fixed Date (and also old Rational) marshaling but not Time.

@headius
Copy link
Member

@headius headius commented Sep 22, 2013

If you make the test changes to jruby/ruby on the appropriate branch (jruby-ruby_1_9_3 most likely) that is probably an ok way to keep them around. We probably need a better system here, but it is what it is.

Once we're caught up, I can see about merging some of our test changes to MRI proper.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants