Remove exception from sun.rmi.runtime.Log#getSource()

[Mandy Chung]

This patch looks okay to me.

Mandy

On 8/23/19 8:44 AM, Roger Riggs wrote:
> Hi Philippe,
>
> The change looks good, I'll run it through our tests and push if its 
> all ok.
> (Pending anyone else's comments.)
>
> On 8/23/19 10:38 AM, Philippe Marschall wrote:
>>
>>
>> On 22.08.19 19:56, Mandy Chung wrote:
>>> Hi Philippe,
>>>
>>> This is a good use of StackWalker.   getSource can simply return
>>> StackFrame and avoid the creation of String[].
>>
>> Updated webrev [1] and inline diff at the end.
>>
>> Something I noted is that we could replace
>> sun.rmi.runtime.Log.LogStreamLog.unqualifiedName(String) with
>> Class#getSimpleName() which is cached since JDK-8187123. That would
>> remove an additional bit of allocation. However only in the pre-1.4
>> logging case, which I assume is uncommon. In addition that would require
>> us to create a StackWalker with Option#RETAIN_CLASS_REFERENCE. If we do
>> not want to do this in the jul case we would have to create a LogFactory
>> specific #getSource implementation with a different StackWalker. I am
>> not sure this is worth it.
>>
>>> Stuart will give you better guidance related to RMI testing.
>>>
>>> I see that test/jdk/sun/rmi/runtime/Log has a few RMI logging tests.
>>> RMI tests are in tier3.  You can run jdk_rmi test group to verify
>>> this patch.
>>
>> I am new to this, I did
>>
>> make run-test TEST=jdk_rmi
>>
>> and all tests passed.
> That's a start, running the complete suite of tests may catch 
> something else or another dependency outside of the specific rmi tests.
>>
>>> I notice that pre-1.4 RMI logging support.  I wonder if this is
>>> time to remove it.
>>
>> I would love to do this but I would assume that requires at least a
>> different issue and probably a change note that the property and feature
>> are gone. I assume deprecating java.rmi.server.LogStream for removal is
>> not an option.
> Right, that's a separate issue and takes work and process to remove.
>>
>>
>> I'll be away from a keyboard for a week so I won't be able to answer.
>
> ok, Enjoy
>
> Thanks, Roger
>
>>
>>
>> --- old/src/java.rmi/share/classes/sun/rmi/runtime/Log.java 2019-08-23
>> 15:43:28.452810308 +0200
>> +++ new/src/java.rmi/share/classes/sun/rmi/runtime/Log.java 2019-08-23
>> 15:43:28.320811517 +0200
>> @@ -28,8 +28,10 @@
>>  import java.io.ByteArrayOutputStream;
>>  import java.io.PrintStream;
>>  import java.io.OutputStream;
>> +import java.lang.StackWalker.StackFrame;
>>  import java.rmi.server.LogStream;
>>  import java.security.PrivilegedAction;
>> +import java.util.Set;
>>  import java.util.logging.Handler;
>>  import java.util.logging.SimpleFormatter;
>>  import java.util.logging.Level;
>> @@ -62,6 +64,8 @@
>>      public static final Level BRIEF = Level.FINE;
>>      public static final Level VERBOSE = Level.FINER;
>>
>> +    private static final StackWalker WALKER =
>> StackWalker.getInstance(Set.of(), 4);
>> +
>>      /* selects log implementation */
>>      private static final LogFactory logFactory;
>>      static {
>> @@ -217,16 +221,16 @@
>>
>>          public void log(Level level, String message) {
>>              if (isLoggable(level)) {
>> -                String[] source = getSource();
>> -                logger.logp(level, source[0], source[1],
>> +                StackFrame sourceFrame = getSource();
>> +                logger.logp(level, sourceFrame.getClassName(),
>> sourceFrame.getMethodName(),
>>                             Thread.currentThread().getName() + ": " +
>> message);
>>              }
>>          }
>>
>>          public void log(Level level, String message, Throwable 
>> thrown) {
>>              if (isLoggable(level)) {
>> -                String[] source = getSource();
>> -                logger.logp(level, source[0], source[1],
>> +                StackFrame sourceFrame = getSource();
>> +                logger.logp(level, sourceFrame.getClassName(),
>> sourceFrame.getMethodName(),
>>                      Thread.currentThread().getName() + ": " +
>>                             message, thrown);
>>              }
>> @@ -390,9 +394,9 @@
>>
>>          public void log(Level messageLevel, String message) {
>>              if (isLoggable(messageLevel)) {
>> -                String[] source = getSource();
>> -                stream.println(unqualifiedName(source[0]) +
>> -                               "." + source[1] + ": " + message);
>> +                StackFrame sourceFrame = getSource();
>> +
>> stream.println(unqualifiedName(sourceFrame.getClassName()) +
>> +                               "." + sourceFrame.getMethodName() + ": "
>> + message);
>>              }
>>          }
>>
>> @@ -403,9 +407,9 @@
>>                   * RemoteServer.getLog
>>                   */
>>                  synchronized (stream) {
>> -                    String[] source = getSource();
>> -                    stream.println(unqualifiedName(source[0]) + "." +
>> -                                   source[1] + ": " + message);
>> +                    StackFrame sourceFrame = getSource();
>> +
>> stream.println(unqualifiedName(sourceFrame.getClassName()) + "." +
>> +                                    sourceFrame.getMethodName() + ": "
>> + message);
>>                      thrown.printStackTrace(stream);
>>                  }
>>              }
>> @@ -441,13 +445,12 @@
>>      }
>>
>>      /**
>> -     * Obtain class and method names of code calling a log method.
>> +     * Obtain stack frame of code calling a log method.
>>       */
>> -    private static String[] getSource() {
>> -        StackTraceElement[] trace = (new Exception()).getStackTrace();
>> -        return new String[] {
>> -            trace[3].getClassName(),
>> -            trace[3].getMethodName()
>> -        };
>> +    private static StackFrame getSource() {
>> +        return WALKER.walk(s -> s
>> +                                 .skip(3)
>> +                                 .findFirst()
>> +                                 .get());
>>      }
>>  }
>>
>>  [1]
>> https://github.com/marschall/webrevs/raw/master/Log-getSource-02/webrev.zip 
>>
>>
>> Philippe
>