[bg-jug] Re: RFR: 5050783: Throwable convenience method: String getStackTraceString()
Claes Redestad
claes.redestad at oracle.com
Sun Jan 18 22:20:50 UTC 2015
Hi,
On 2015-01-16 21:14, Yavor Nikolov wrote:
> Hi,
>
>> On that note, I'm thinking loudly about whether or not it'd be appropriate
>> to extract common methods in PrintStream and PrintWriter to an interface.
>> java.io.Printable?
>>
> Thinking a bit further in a similar direction: a thinner interface gives
> more clarity about the API contract, it's also easier to override,
> especially if eligible for lambdas. I speculatively guess that logging
> implementations might also benefit from that: I mean simpler API like
> printStackTrace(Printable).
Agreed, but sadly we wouldn't able to remove any existing methods
(breaks binary compatibility), thus adding methods taking Printable
to legacy classes like Throwable might cost more complexity in the
API than it will help to reduce. A more thorough analysis might be
needed to prove if it's worth it.
The behavior here should be pretty well-defined, so I've so far not
seen any purely theoretical reasons not to do it (should not violate
the Liskov substition principle etc).
Related discussions have occurred on core-libs before, for example
adding methods with ones taking CharSequence next to or even
instead of ones taking String[1]. Typically this hasn't happened
for various reasons, but it seems easier to use the more generic
interface when adding new functionality[2].
> Regarding the Appendable idea: one concern has been that println(s) and
> append(x + NL) have different behaviour in case of auto-flush-enabled
> output. Do we need to bother preserving printStackTrace auto-flush
> behaviour? (The Javadoc doesn't specify it, yet in real world there might
> have been assumptions that it auto-flushes...).
Yes, sufficient reason to scrap that approach.
> Best regards,
> Yavor
>
> On Thu, Jan 15, 2015 at 3:05 PM, Ivan St. Ivanov<ivan.st.ivanov at gmail.com>
> wrote:
>
>> Thanks everybody for the feedback! :)
>>
>> Claes, we were deliberately trying to avoid the "Apache commons" solution.
>> It looks perfectly fine outside of the JDK. But our wild guess was that as
>> we have access to the Throwable private methods, we can do something more
>> elegant. The fact is (and I shared it with you in our private conversation)
>> that we now understood that most elegant is not always the best when it
>> comes to the core of the core of Java :)
>>
>> On that note, I'm thinking loudly about whether or not it'd be
>> appropriate to extract common methods in PrintStream and
>> PrintWriter to an interface. java.io.Printable?
>>
>> This would allow getting rid of most inner classes in j.l.Throwable
>> along with some duplicated code across the JDK (I've noticed
>> at least a few places where there are identical methods which
>> differ only in that one takes a PrintStream, the other a PrintWriter).
>>
>> Do you think that we can experiment with that as part of the effort for
>> this issue, or you think it is better to have it as a separate work item
>> (possibly done by someone with more experience) and apply it here once it
>> is ready?
For this particular issue, no, this is most definitely out of scope. Working
on larger API changes (cleanups?) like this should probably be done, planned
and worked on as a (smallish) JEP[3], so that the pros and cons can be
made clear.
Whether or not this is interesting: I don't really know. I think it
could be of
interest if you're willing to spend the time, but I'm far from the only
one who
needs to be convinced.
There might be more approachable issues to work on to get more experience
and contribute more directly to the project. Hint: anything which
doesn't require a
public API change is typically orders of magnitude easier to get accepted.
/Claes
[1]
http://mail.openjdk.java.net/pipermail/core-libs-dev/2012-April/009819.html
[2] https://bugs.openjdk.java.net/browse/JDK-8041972
[3] http://cr.openjdk.java.net/~mr/jep/jep-2.0.html
>>
>> Regards,
>> Ivan
>>
>>
>> On Thu, Jan 15, 2015 at 2:34 PM, David Holmes<david.holmes at oracle.com>
>> wrote:
>>
>>> On 15/01/2015 6:34 AM, Ivan St. Ivanov wrote:
>>>
>>>> Hi Remi,
>>>>
>>>> I tried it and, well, the JDK image now does not even compile classes
>>>> that
>>>> contain lambda expressions:
>>>>
>>>> javac -cp . ./LambdaMetaFactoryTester.java
>>>> Exception in thread "main"
>>>> Exception: java.lang.BootstrapMethodError thrown from the
>>>> UncaughtExceptionHandler in thread "main"
>>>>
>>>> With the old Throwable implementation it printed a long stack trace of
>>>> exceptions, but at least my sample class was compiled.
>>>>
>>>> So, I have two questions:
>>>> 1) Does anyone dump stack traces in such location, even for debugging
>>>> purposes?
>>>> 2) If yes, what are the other JDK classes and methods where we should
>>>> avoid
>>>> using lambdas?
>>>>
>>> It is hard to be definitive, but the initialization sequence for the JDK
>>> classes is extremely fragile. So anything that gets initialized very early
>>> in the VM initialization sequence needs to avoid using features that would
>>> require that the initialization sequence change. Using -verbose:class shows
>>> the order in which classes get loaded.
>>>
>>> David
>>>
>>>
>>> Thanks,
>>>> Ivan
>>>>
>>>> On Wed, Jan 14, 2015 at 9:44 PM, Remi Forax<forax at univ-mlv.fr> wrote:
>>>>
>>>> While using a lambda is cool in that context,
>>>>> if someone tries to do a printStackTrace in the LambdaMetafactory (for
>>>>> debugging purpose by example),
>>>>> it will be fun to debug.
>>>>>
>>>>> cheers,
>>>>> Rémi
>>>>>
>>>>>
>>>>> On 01/14/2015 08:25 PM, Ivan St. Ivanov wrote:
>>>>>
>>>>> Hi Claes, core libs developers,
>>>>>> As promised, we did yesterday our hackathon in the Bulgarian Java User
>>>>>> Group and incorporated the feedback that we got from you. Here is our
>>>>>> next
>>>>>> webrev:
>>>>>>
>>>>>> http://dmitryalexandrov.net/~bgjug/5050783/webrev.01/
>>>>>>
>>>>>> As you can see:
>>>>>>
>>>>>> - We got rid of the inner classes altogether in favor of
>>>>>> java.util.function.Consumer
>>>>>> - The javadoc was updated to follow the standards [1]
>>>>>> - In the tests we use an exception that we create rather than relying
>>>>>> on
>>>>>> one from the JDK that potentially could change
>>>>>>
>>>>>> We also ran some microbenchmarks to compare the performance of the new
>>>>>> functionality with the Apache commons solution mentioned in the bug.
>>>>>> Here
>>>>>> is the benchmark test:
>>>>>>
>>>>>> @State(Scope.Benchmark)
>>>>>> public class ThrowableStacktraceBenchmark {
>>>>>> Throwable chainedThrowable;
>>>>>> Throwable simpleThrowable;
>>>>>>
>>>>>> @Setup
>>>>>> public void prepare() {
>>>>>> chainedThrowable = createThrowable(64);
>>>>>> simpleThrowable = new Throwable("Simple Throwable");
>>>>>> }
>>>>>>
>>>>>> Throwable createThrowable(int depthLevel) {
>>>>>> Throwable t = null;
>>>>>> for (int i = 0; i < depthLevel; ++i) {
>>>>>> t = new Throwable("My dummy Throwable " + i, t);
>>>>>> }
>>>>>> return t;
>>>>>> }
>>>>>>
>>>>>> @Benchmark
>>>>>> public void benchmarkThrowableGetStackTraceString() {
>>>>>> chainedThrowable.getStackTraceString();
>>>>>> }
>>>>>>
>>>>>> @Benchmark
>>>>>> public void benchmarkThrowableGetStackTraceStringSmall() {
>>>>>> simpleThrowable.getStackTraceString();
>>>>>> }
>>>>>>
>>>>>> @Benchmark
>>>>>> public void benchmarkThrowablePrintStackTraceToStringWriter() {
>>>>>> StringWriter sw = new StringWriter();
>>>>>> chainedThrowable.printStackTrace(new PrintWriter(sw));
>>>>>> sw.toString();
>>>>>> }
>>>>>>
>>>>>> @Benchmark
>>>>>> public void benchmarkThrowablePrintStackTraceToStringWriterSmall()
>>>>>> {
>>>>>> StringWriter sw = new StringWriter();
>>>>>> simpleThrowable.printStackTrace(new PrintWriter(sw));
>>>>>> sw.toString();
>>>>>> }
>>>>>> }
>>>>>>
>>>>>> And here are the results (test was run with the following options -wi
>>>>>> 5 -i
>>>>>> 5 -t 1):
>>>>>>
>>>>>> Benchmark Mode Samples Score Error Units
>>>>>> o.m.t.ThrowableStacktraceBenchmark.benchmarkThrowableGetStackTrac
>>>>>> eString
>>>>>> thrpt 50 25351.743 ± 347.326 ops/s
>>>>>> o.m.t.ThrowableStacktraceBenchmark.benchmarkThrowableGetStackTrac
>>>>>> eStringSmall
>>>>>> thrpt 50 134990.643 ± 760.225 ops/s
>>>>>> o.m.t.ThrowableStacktraceBenchmark.benchmarkThrowablePrintStackTr
>>>>>> aceToStringWriter
>>>>>> thrpt 50 22730.786 ± 167.378 ops/s
>>>>>> o.m.t.ThrowableStacktraceBenchmark.benchmarkThrowablePrintStackTr
>>>>>> aceToStringWriterSmall
>>>>>> thrpt 50 115223.025 ± 936.770 ops/s
>>>>>>
>>>>>> Just in any case we ran this test, actually just the last two methods,
>>>>>> with
>>>>>> JDK 1.9 ea build 45. We wanted to compare the result of the Apache
>>>>>> commons
>>>>>> solution with inner classes (as it is now) to the same, but with
>>>>>> java.util.function.Consumer. There are the results with the inner
>>>>>> classes:
>>>>>>
>>>>>> o.m.t.ThrowableStacktraceBenchmark.benchmarkThrowablePrintStackTr
>>>>>> aceToStringWriter
>>>>>> thrpt 50 22974.324 ± 391.564 ops/s
>>>>>> o.m.t.ThrowableStacktraceBenchmark.benchmarkThrowablePrintStackTr
>>>>>> aceToStringWriterSmall
>>>>>> thrpt 50 114973.452 ± 1051.276 ops/s
>>>>>>
>>>>>> So you can see that the performance is nearly the same (I guess that
>>>>>> the
>>>>>> difference is ignorable).
>>>>>>
>>>>>> Please share with us the feedback about this proposal!
>>>>>>
>>>>>> Thanks,
>>>>>> Ivan
>>>>>>
>>>>>> [1]http://www.oracle.com/technetwork/articles/java/index-137868.html
>>>>>>
>>>>>> On Wed, Dec 31, 2014 at 3:32 PM, Claes Redestad <
>>>>>> claes.redestad at oracle.com>
>>>>>> wrote:
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>>> seems like a reasonable convenience method. :-)
>>>>>>>
>>>>>>> Process stuff: OCA/JSPA signing aside, the patch needs to be
>>>>>>> submitted in
>>>>>>> its
>>>>>>> entirety to some part of the OpenJDK infrastructure before it can be
>>>>>>> accepted.
>>>>>>> Attaching the patch in an e-mail to this mailing list is acceptable
>>>>>>> for a
>>>>>>> small
>>>>>>> patch like this when lacking upload privileges to cr.openjdk.java.net.
>>>>>>> A
>>>>>>> sponsor
>>>>>>> could help you with the uploading etc.
>>>>>>>
>>>>>>> This request adds a new public method, which means some internal
>>>>>>> approval
>>>>>>> will be necessary. This is something you'll need help with from an
>>>>>>> Oracle
>>>>>>> sponsor. I could volunteer to initiate the requests internally.
>>>>>>>
>>>>>>> Code feedback:
>>>>>>>
>>>>>>> The test relies on the message generated when provoking a division by
>>>>>>> zero. I'm not sure this is a good idea nor the place to enforce this
>>>>>>> message format with a test. Perhaps just throwing a RuntimeException
>>>>>>> with some specified message and test for that?
>>>>>>>
>>>>>>> May I suggest to also add a test to ensure the method behaves well
>>>>>>> for an
>>>>>>> exception with fillInStackTrace overridden to produce stackless
>>>>>>> exceptions?
>>>>>>>
>>>>>>> Just a thought... I noticed PrintWriter, PrintStream and StringBuilder
>>>>>>> all
>>>>>>> inherit from Appendable, so the PrintStreamOrWriter/StackTracePrinter
>>>>>>> classes
>>>>>>> could be replaced with a single StackTraceAppendable taking an
>>>>>>> Appendable.
>>>>>>> One
>>>>>>> static class instead of 1 abstract and 3 concrete static inner classes
>>>>>>> could work
>>>>>>> out to be a good deal, but there's some odd mechanics in
>>>>>>> BufferedWriter/PrintWriter
>>>>>>> to use the value of the line.separator property at object creation
>>>>>>> time
>>>>>>> which
>>>>>>> would be hard to support without exposing the lineSeparator fields to
>>>>>>> Throwable.
>>>>>>> Perhaps a future consideration.
>>>>>>>
>>>>>>> Performance characteristics might change ever so slightly either way,
>>>>>>> so
>>>>>>> it
>>>>>>> would be nice with some microbenchmarks to quantify.
>>>>>>>
>>>>>>> /Claes
>>>>>>>
>>>>>>>
>>>>>>> On 2014-12-31 11:51, Ivan St. Ivanov wrote:
>>>>>>>
>>>>>>> Hello,
>>>>>>>
>>>>>>>> I am Ivan from the Bulgarian Java User Group. As part of our Adopt
>>>>>>>> OpenJDK
>>>>>>>> efforts, we organized hackathon beginning of this month in Sofia.
>>>>>>>> There
>>>>>>>> we
>>>>>>>> worked on the following JIRA issue:
>>>>>>>>
>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-5050783
>>>>>>>>
>>>>>>>> Unfortunately none of us has author privileges, so the issue is still
>>>>>>>> unassigned. But, nevertheless, our JUG has already signed JSPA and
>>>>>>>> some
>>>>>>>> of
>>>>>>>> our members (including myself) have signed OCA. And we informed in a
>>>>>>>> special email this mailing list about our plans prior to the meeting.
>>>>>>>>
>>>>>>>> Our hackathon was already documented by our JUG member Mihail
>>>>>>>> Stoynov:
>>>>>>>>
>>>>>>>> http://mihail.stoynov.com/2014/12/12/building-openjdk-
>>>>>>>> and-submitting-a-solution-on-a-feature-request-with-the-
>>>>>>>> bulgarian-java-user-group/
>>>>>>>>
>>>>>>>> We used the days after the meeting for offline polishing our
>>>>>>>> contribution
>>>>>>>> and adding some more unit tests. Here is the result of our work:
>>>>>>>>
>>>>>>>> http://dmitryalexandrov.net/~bgjug/5050783/webrev.00/
>>>>>>>>
>>>>>>>> We would be very happy if you are able to review it, share your
>>>>>>>> feedback
>>>>>>>> and guide us through the contribution process as this is our first
>>>>>>>> attempt.
>>>>>>>>
>>>>>>>> Thanks and have a very successful year!
>>>>>>>> Ivan
>>>>>>>>
>>>>>>>>
>>>>>>>>
>> --
>> You received this message because you are subscribed to the Google Groups
>> "Bulgarian Java Users Group" group.
>> To unsubscribe from this group and stop receiving emails from it, send an
>> email tobg-jug+unsubscribe at googlegroups.com.
>> To post to this group, send email tobg-jug at googlegroups.com.
>> Visit this group athttp://groups.google.com/group/bg-jug.
>> For more options, visithttps://groups.google.com/d/optout.
>>
More information about the core-libs-dev
mailing list