[+jjb]  (Please keep Josh in this thread - he is the primary author)<br><br><br><div class="gmail_quote">On Tue, Jul 7, 2009 at 02:51, Christopher Hegarty -Sun Microsystems Ireland <span dir="ltr"><<a href="mailto:Christopher.Hegarty@sun.com">Christopher.Hegarty@sun.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">Hi Martin,<br>
<br>
Sorry for joining the party late...<br>
<br>
I think adding the system property should take care of the compatibility issues, at least giving the user the ability to revert to the old behavior if they so choose.<br>
<br>
I have a few minor comments ( if these issue have been discussed already I apologize ):<br>
<br>
1) Should we update the Arrays class description and appropriate sort<br>
   methods to now refer to timsort instead of saying: "The sorting<br>
   algorithm is a modified mergesort...". I know this is not strictly<br>
   necessary, but you must have considered it already, right?</blockquote><div><br>Josh?<br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<br>
2) With the addition of @throws IllegalArgumentException, this<br>
   condition cannot be met with the old merge sort right, i.e. running<br>
   with -Djava.util.Arrays.useLegacyMergeSort=true. So we're saying<br>
   that all bets are off when running with this property set?</blockquote><div><br>No.  Please re-read the @throws IllegalArgumentException.<br>It is carefully worded to make no promises at all.  All bets are off - period.<br>
No JCK tests can be written or are invalidated.<br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><br>
3) Have I missed something obvious! Can the test run, or even compile?<br>
   Sorter.java, L29:<br>
     ComparableTimSort.sort(array);<br>
</blockquote><div><br>These are not tests.<br>Please see:<br><a href="http://cr.openjdk.java.net/~martin/webrevs/openjdk7/timsort/raw_files/new/test/java/util/TimSort/README">http://cr.openjdk.java.net/~martin/webrevs/openjdk7/timsort/raw_files/new/test/java/util/TimSort/README</a><br>
<br>One could rework the benchmarks to work with the newly introduced<br>system property (using reflection to invoke appropriate methods)<br>but I have no plans to do that.<br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<br>
BTW, if you agree, I think we should stick with the process as is today for making spec changes. Once agreed, I can submit a CCC request for this change and help with the communication to get it approved.<br>
</blockquote><div><br>OK.  Keep in mind that the spec changes are almost no-ops.<br><br>Martin<br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<br>
-Chris.<div><div></div><div class="h5"><br>
<br>
<br>
<br>
<br>
Martin Buchholz wrote:<br>
<blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
No one actually said NO, but both Alan and Andrew strongly hinted that<br>
a backward compatibility mode would be good.<br>
So I kept the old implementation and allow it to be selectable<br>
via a system property.  There's nothing more compatible than<br>
the legacy implementation.<br>
<br>
    /**<br>
     * Old merge sort implementation can be selected (for<br>
     * compatibility with broken comparators) using a system property.<br>
     * Cannot be a static boolean in the enclosing class due to<br>
     * circular dependencies.  To be removed in a future release.<br>
     */<br>
    static final class LegacyMergeSort {<br>
        private static final boolean userRequested =<br>
            java.security.AccessController.doPrivileged(<br>
                new sun.security.action.GetBooleanAction(<br>
                    "java.util.Arrays.useLegacyMergeSort")).booleanValue();<br>
    }<br>
<br>
The sort method bodies now look like:<br>
<br>
        if (LegacyMergeSort.userRequested)<br>
            legacyMergeSort(a);<br>
        else<br>
            ComparableTimSort.sort(a);<br>
<br>
New webrev at:<br>
<a href="http://cr.openjdk.java.net/%7Emartin/webrevs/openjdk7/timsort/" target="_blank">http://cr.openjdk.java.net/~martin/webrevs/openjdk7/timsort/</a><br>
<br>
If I don't hear from anyone soon (e.g. for CCC approval)<br>
I'll commit to the tl forest.<br>
<br>
Martin<br>
<br>
On Tue, Jun 30, 2009 at 11:28, Alan Bateman<<a href="mailto:Alan.Bateman@sun.com" target="_blank">Alan.Bateman@sun.com</a>> wrote:<br>
<blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
Martin Buchholz wrote:<br>
<blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
:<br>
<br>
As you suggested, I added the Classpath exception wording<br>
to TimSort.java and ComparableTimSort.java.<br>
<br>
I excised the old mergesort code from Arrays.java.<br>
<br>
webrev regenerated.<br>
</blockquote>
Thanks for doing this.<br>
<blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
Adding all-or-nothing wording would be good to add,<br>
perhaps to the class-level javadoc.  But it doesn't<br>
have to be part of this change.<br>
</blockquote>
I brought it up because it looks like (and you can correct me) that the IAE<br>
may be thrown after some reordering of the array has taken place. This might<br>
be unexpected.<br>
<br>
<blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
The JDK project has unusually high compatibility concerns.<br>
I and Josh believe the introduction of a possible IAE if the<br>
comparator doesn't satisfy its contract is the right thing,<br>
but we'd also be willing to change the code to swallow the IAE<br>
or to do so conditionally based on the value of a system property.<br>
Keep in mind that a call to foreign code can throw any throwable at all,<br>
so a rogue comparator can cause arbitrary behavior even today.<br>
</blockquote>
I think most people would agree that that catching these otherwise-silent<br>
failures is a good thing. The problem is the customer that upgrades the JDK<br>
on a production system and gets burnt by some broken third party code that<br>
"used" to work. Having some way to restore existing behavior would make this<br>
an easier sell. The suggestion from Jason to change it to an assertion might<br>
be worth thinking about too but I suspect that few developers test with<br>
assertions enabled.<br>
<br>
-Alan.<br>
<br>
</blockquote></blockquote>
</div></div></blockquote></div><br>