Remove intermediate Map for Hash directly. This was pretty minor chan…

…ge and it also gave a modest improvement (BEFORE): ```text jruby ../snippets/date_bench.rb Warming up -------------------------------------- _strptime 24.108k i/100ms Calculating ------------------------------------- _strptime 291.972k (± 6.1%) i/s - 2.917M in 10.031620s ``` AFTER ```text jruby ../snippets/date_bench.rb Warming up -------------------------------------- _strptime 26.267k i/100ms Calculating ------------------------------------- _strptime 319.069k (± 5.8%) i/s - 3.178M in 9.998810s ```
jruby · Jun 19, 2017 · 65e1495 · 65e1495
1 parent 4323f93
commit 65e1495
Showing 2 changed files with 51 additions and 90 deletions.
diff --git a/core/src/main/java/org/jruby/ext/date/RubyDate.java b/core/src/main/java/org/jruby/ext/date/RubyDate.java
@@ -30,9 +30,7 @@
  ***** END LICENSE BLOCK *****/
 package org.jruby.ext.date;
 
-import org.jruby.RubyHash;
 import org.jruby.RubyString;
-import org.jruby.RubySymbol;
 import org.jruby.anno.JRubyMethod;
 import org.jruby.anno.JRubyModule;
 import org.jruby.runtime.ThreadContext;
@@ -41,8 +39,6 @@
 import org.jruby.util.RubyDateParser;
 import org.jruby.util.TypeConverter;
 
-import java.util.Map;
-
 @JRubyModule(name = "Date")
 public class RubyDate {
     private static final ByteList DEFAULT_FORMAT = new ByteList(new byte[] {'%', 'F'});
@@ -55,18 +51,7 @@ public static IRubyObject _strptime(ThreadContext context, IRubyObject string, I
         RubyString stringString = (RubyString) TypeConverter.checkStringType(context.runtime, string);
         RubyString formatString = (RubyString) TypeConverter.checkStringType(context.runtime, format);
 
-        Map<String, Object> map = new RubyDateParser().parse(context, formatString, stringString);
-        // FIXME: Remove null as return
-        if (map == null) return context.nil;
-
-        RubyHash hash = RubyHash.newHash(context.runtime);
-
-        for (Map.Entry<String, Object> pair: map.entrySet()) {
-            // FIXME: This is using JI for object coercion so we should return ruby types in value
-            hash.put(RubySymbol.newSymbol(context.runtime, pair.getKey()), pair.getValue());
-        }
-
-        return hash;
+        return new RubyDateParser().parse(context, formatString, stringString);
     }
 
     @JRubyMethod(meta = true, required = 1, optional = 1)

diff --git a/core/src/main/java/org/jruby/util/RubyDateParser.java b/core/src/main/java/org/jruby/util/RubyDateParser.java
@@ -25,13 +25,16 @@
  ***** END LICENSE BLOCK *****/
 package org.jruby.util;
 
+import org.jruby.Ruby;
 import org.jruby.RubyBignum;
 import org.jruby.RubyFixnum;
+import org.jruby.RubyHash;
 import org.jruby.RubyRational;
 import org.jruby.RubyString;
+import org.jruby.RubySymbol;
 import org.jruby.runtime.ThreadContext;
+import org.jruby.runtime.builtin.IRubyObject;
 
-import java.util.HashMap;
 import java.util.List;
 
 import static org.jruby.util.StrptimeParser.FormatBag.has;
@@ -50,102 +53,75 @@ public RubyDateParser() {
      * Date._strptime method in JRuby 9.1.5.0's lib/ruby/stdlib/date/format.rb is replaced
      * with this method. This is Java implementation of date__strptime method in MRI 2.3.1's
      * ext/date/date_strptime.c.
-     * @see https://github.com/jruby/jruby/blob/036ce39f0476d4bd718e23e64caff36bb50b8dbc/lib/ruby/stdlib/date/format.rb
-     * @see https://github.com/ruby/ruby/blob/394fa89c67722d35bdda89f10c7de5c304a5efb1/ext/date/date_strptime.c
+     * See:
+     *  https://github.com/jruby/jruby/blob/036ce39f0476d4bd718e23e64caff36bb50b8dbc/lib/ruby/stdlib/date/format.rb
+     *  https://github.com/ruby/ruby/blob/394fa89c67722d35bdda89f10c7de5c304a5efb1/ext/date/date_strptime.c
      */
 
-    public HashMap<String, Object> parse(ThreadContext context, final RubyString format, final RubyString text) {
-        final boolean tainted = text.isTaint();
+    public IRubyObject parse(ThreadContext context, final RubyString format, final RubyString text) {
         final List<StrptimeToken> compiledPattern = strptimeParser.compilePattern(format.asJavaString());
         final StrptimeParser.FormatBag bag = strptimeParser.parse(compiledPattern, text.asJavaString());
-        if (bag != null) {
-            return convertFormatBagToHash(context, bag, tainted);
-        } else {
-            return null;
-        }
+
+        return bag == null ? context.nil :  convertFormatBagToHash(context, bag, text.isTaint());
     }
 
-    private HashMap<String, Object> convertFormatBagToHash(ThreadContext context, StrptimeParser.FormatBag bag, boolean tainted) {
-        final HashMap<String, Object> map = new HashMap<>();
+    private IRubyObject convertFormatBagToHash(ThreadContext context, StrptimeParser.FormatBag bag, boolean tainted) {
+        Ruby runtime = context.runtime;
+        RubyHash hash = RubyHash.newHash(runtime);
+
+        if (has(bag.getMDay())) hash.op_aset(context, RubySymbol.newSymbol(runtime, "mday"), runtime.newFixnum(bag.getMDay()));
+        if (has(bag.getWDay())) hash.op_aset(context, RubySymbol.newSymbol(runtime, "wday"), runtime.newFixnum(bag.getWDay()));
+        if (has(bag.getCWDay())) hash.op_aset(context, RubySymbol.newSymbol(runtime, "cwday"), runtime.newFixnum(bag.getCWDay()));
+        if (has(bag.getYDay())) hash.op_aset(context, RubySymbol.newSymbol(runtime, "yday"), runtime.newFixnum(bag.getYDay()));
+        if (has(bag.getCWeek())) hash.op_aset(context, RubySymbol.newSymbol(runtime, "cweek"), runtime.newFixnum(bag.getCWeek()));
+        if (has(bag.getCWYear())) hash.op_aset(context, RubySymbol.newSymbol(runtime, "cwyear"), RubyBignum.newBignum(runtime, bag.getCWYear()));
+        if (has(bag.getMin())) hash.op_aset(context, RubySymbol.newSymbol(runtime, "min"), runtime.newFixnum(bag.getMin()));
+        if (has(bag.getMon())) hash.op_aset(context, RubySymbol.newSymbol(runtime, "mon"), runtime.newFixnum(bag.getMon()));
+        if (has(bag.getHour())) hash.op_aset(context, RubySymbol.newSymbol(runtime, "hour"), runtime.newFixnum(bag.getHour()));
+        if (has(bag.getYear())) hash.op_aset(context, RubySymbol.newSymbol(runtime, "year"), RubyBignum.newBignum(runtime, bag.getYear()));
+        if (has(bag.getSec())) hash.op_aset(context, RubySymbol.newSymbol(runtime, "sec"), runtime.newFixnum(bag.getSec()))
+                ;
+        if (has(bag.getWNum0())) hash.op_aset(context, RubySymbol.newSymbol(runtime, "wnum0"), runtime.newFixnum(bag.getWNum0()));
+        if (has(bag.getWNum1())) hash.op_aset(context, RubySymbol.newSymbol(runtime, "wnum1"), runtime.newFixnum(bag.getWNum1()));
 
-        if (has(bag.getMDay())) {
-            map.put("mday", bag.getMDay());
-        }
-        if (has(bag.getWDay())) {
-            map.put("wday", bag.getWDay());
-        }
-        if (has(bag.getCWDay())) {
-            map.put("cwday", bag.getCWDay());
-        }
-        if (has(bag.getYDay())) {
-            map.put("yday", bag.getYDay());
-        }
-        if (has(bag.getCWeek())) {
-            map.put("cweek", bag.getCWeek());
-        }
-        if (has(bag.getCWYear())) {
-            map.put("cwyear", bag.getCWYear());
-        }
-        if (has(bag.getMin())) {
-            map.put("min", bag.getMin());
-        }
-        if (has(bag.getMon())) {
-            map.put("mon", bag.getMon());
-        }
-        if (has(bag.getHour())) {
-            map.put("hour", bag.getHour());
-        }
-        if (has(bag.getYear())) {
-            map.put("year", bag.getYear());
-        }
-        if (has(bag.getSec())) {
-            map.put("sec", bag.getSec());
-        }
-        if (has(bag.getWNum0())) {
-            map.put("wnum0", bag.getWNum0());
-        }
-        if (has(bag.getWNum1())) {
-            map.put("wnum1", bag.getWNum1());
-        }
         if (bag.getZone() != null) {
-            final RubyString zone = RubyString.newString(context.getRuntime(), bag.getZone());
-            if (tainted) {
-                zone.taint(context);
-            }
-            map.put("zone", zone);
+            final RubyString zone = RubyString.newString(runtime, bag.getZone());
+            if (tainted) zone.taint(context);
+
+            hash.op_aset(context, RubySymbol.newSymbol(runtime, "zone"), zone);
             int offset = TimeZoneConverter.dateZoneToDiff(bag.getZone());
-            if (offset != Integer.MIN_VALUE) {
-                map.put("offset", offset);
-            }
+            if (offset != Integer.MIN_VALUE) hash.op_aset(context, RubySymbol.newSymbol(runtime, "offset"), runtime.newFixnum(offset));
         }
+
         if (has(bag.getSecFraction())) {
-            final RubyBignum secFraction = RubyBignum.newBignum(context.getRuntime(), bag.getSecFraction());
-            final RubyFixnum secFractionSize = RubyFixnum.newFixnum(context.getRuntime(), (long)Math.pow(10, bag.getSecFractionSize()));
-            map.put("sec_fraction", RubyRational.newRationalCanonicalize(context, secFraction, secFractionSize));
+            final RubyBignum secFraction = RubyBignum.newBignum(runtime, bag.getSecFraction());
+            final RubyFixnum secFractionSize = RubyFixnum.newFixnum(runtime, (long)Math.pow(10, bag.getSecFractionSize()));
+            hash.op_aset(context, RubySymbol.newSymbol(runtime, "sec_fraction"),
+                    RubyRational.newRationalCanonicalize(context, secFraction, secFractionSize));
         }
+
         if (bag.has(bag.getSeconds())) {
             if (has(bag.getSecondsSize())) {
-                final RubyBignum seconds = RubyBignum.newBignum(context.getRuntime(), bag.getSeconds());
-                final RubyFixnum secondsSize = RubyFixnum.newFixnum(context.getRuntime(), (long)Math.pow(10, bag.getSecondsSize()));
-                map.put("seconds", RubyRational.newRationalCanonicalize(context, seconds, secondsSize));
+                final RubyBignum seconds = RubyBignum.newBignum(runtime, bag.getSeconds());
+                final RubyFixnum secondsSize = RubyFixnum.newFixnum(runtime, (long)Math.pow(10, bag.getSecondsSize()));
+                hash.op_aset(context, RubySymbol.newSymbol(runtime, "seconds"), RubyRational.newRationalCanonicalize(context, seconds, secondsSize));
             } else {
-                map.put("seconds", bag.getSeconds());
+                hash.op_aset(context, RubySymbol.newSymbol(runtime, "seconds"), RubyBignum.newBignum(runtime, bag.getSeconds()));
             }
         }
         if (has(bag.getMerid())) {
-            map.put("_merid", bag.getMerid());
+            hash.op_aset(context, RubySymbol.newSymbol(runtime, "_merid"), runtime.newFixnum(bag.getMerid()));
         }
         if (has(bag.getCent())) {
-            map.put("_cent", bag.getCent());
+            hash.op_aset(context, RubySymbol.newSymbol(runtime, "_cent"), RubyBignum.newBignum(runtime, bag.getCent()));
         }
         if (bag.getLeftover() != null) {
-            final RubyString leftover = RubyString.newString(context.getRuntime(), bag.getLeftover());
-            if (tainted) {
-                leftover.taint(context);
-            }
-            map.put("leftover", leftover);
+            final RubyString leftover = RubyString.newString(runtime, bag.getLeftover());
+            if (tainted) leftover.taint(context);
+
+            hash.op_aset(context, RubySymbol.newSymbol(runtime, "leftover"), leftover);
         }
 
-        return map;
+        return hash;
     }
 }