gwtproject / gwt

GWT Open Source Project
http://www.gwtproject.org
1.51k stars 372 forks source link

java.util.Date.fixDaylightSavings is costly, unnecessary in modern browsers #9739

Open niloc132 opened 2 years ago

niloc132 commented 2 years ago

GWT version: 2.8+ (but goes back to the early days of gwt) Browser (with version): any Operating System: any


Description

GWT's java.util.Date emulation has a check in the method fixDaylightSavings to make sure its dates are consistent with the JVM: https://github.com/gwtproject/gwt/blob/3dc21707a4b6aa1af7139986fa7e3b9c213fee05/user/super/com/google/gwt/emul/java/util/Date.java#L232-L299

This is less applicable today than it used to be: IE8 returns 1 (workaround required) IE9 returns 1 (workaround required) IE10 returns 3 (expected) IE11 returns 3 (expected) Edge returns 3 (expected) Chrome 93 returns 3 (expected) Firefox 91 returns 3 (expected)

GWT's provided DateTimeFormat ends up calling this several times in the course of parsing a string and writing it to a Date object - for example, this is called by setDate/setHours/setMinutes/setSeconds, all of which are called sequentially in the internal DateRecord type that helps with parsing (and setDate is called twice, setMonth once before this snippet): https://github.com/gwtproject/gwt/blob/3dc21707a4b6aa1af7139986fa7e3b9c213fee05/user/src/com/google/gwt/i18n/shared/impl/DateRecord.java#L142-L150

Given that two browsers GWT 2 still supports require this branch, we can't outright remove it (until we remove support for those browsers...), but perhaps a workaround could be added where we detect "old enough" / "new enough" browsers and selectively apply the check.

Steps to reproduce

Visit https://hollow.colinalworth.com/date.html from a computer in a US time zone which used daylight saving time in 2009 and check what number is written to the page - if 3, the browser in use behaves the same as the JVM, if 1, the browser will require a workaround. This page runs the following JS, as noted in the Date implementation above: new Date(2009, 2, 8, 2, 0, 0).getHours().

Here is a small sample app that demonstrates the issue in either GWT2 or J2CL:

    public static final int STRING_ARRAY_SIZE = 1000;
    private DateTimeFormat formatter;
    private String[] dateStrings;
    @Override
    public void onModuleLoad() {
        HTMLInputElement countBox = (HTMLInputElement) document.getElementById("count");
        Element run = document.getElementById("run");
        run.onclick = e -> {
            parse(Integer.parseInt(countBox.value));
            return null;
        };

        prepare();
    }

    /**
     * Make format and test data
     */
    private void prepare() {
        formatter = DateTimeFormat.getFormat("yyyy-MM-dd'T'HH:mm:ss");

        // Always make 1000 strings, we'll just loop over them more if more is requested
        double nextDateInMillis = 0;
        Random random = new Random();
        dateStrings = new String[STRING_ARRAY_SIZE];
        for (int i = 0; i < dateStrings.length; i++) {
            double jitter = random.nextGaussian() * (1000 * 60 * 60 * 24);
            nextDateInMillis += jitter;
            dateStrings[i] = formatter.format(new Date((long) nextDateInMillis));
        }
    }

    private void parse(int count) {
        // only parse to a Date, focus only on the fixDaylightSaving perf
        Date[] dates = new Date[count];
        for (int i = 0; i < count; i++) {
            dates[i] = formatter.parse(dateStrings[i % STRING_ARRAY_SIZE]);
        }

        // keep values around so that compiler won't junk them
        Js.asPropertyMap(window).set("result", dates);
    }

HTML file to run it with:

  <label for="count">Count:</label><input id="count" type="number" value="1000">
  <button id="run">Run</button>

Testing with 100,000 dates to parse, on my computer this takes ~1.3s in either gwt2 or j2cl+closure. Here's the performance data from chrome's dev tools from the GWT2 build of this (using style=PRETTY):

screenshot746 screenshot748

Same details from J2CL+closure (though using ADVANCED compile, and I'm not sure how to keep names unobfuscated in a production build, but the structure should be clear from the above images):

screenshot749 screenshot751

niloc132 commented 2 years ago

Review pending https://gwt-review.googlesource.com/c/gwt/+/23761, but this is waiting on two other patches to be able to land.

niloc132 commented 2 years ago

The fix I proposed doesn't work, I incorrectly verified that modern browsers are behaving by only testing one of the two cased documented as "some browsers have this behavior". I'll try to either simplify the fix and see if there are any other benefits to gain here.