hg: jdk8/tl/langtools: 8022260: Rename javac.code.Annotations to javac.code.SymbolMetadata
Changeset: 6cffcd15a17e Author:jfranck Date: 2013-09-09 09:58 +0200 URL: http://hg.openjdk.java.net/jdk8/tl/langtools/rev/6cffcd15a17e 8022260: Rename javac.code.Annotations to javac.code.SymbolMetadata Reviewed-by: jfranck, jjg Contributed-by: Andreas Lundblad - src/share/classes/com/sun/tools/javac/code/Annotations.java ! src/share/classes/com/sun/tools/javac/code/Symbol.java + src/share/classes/com/sun/tools/javac/code/SymbolMetadata.java ! test/tools/javac/lib/DPrinter.java
Re: RFR: Changes to disable and/or remove Solaris 32-bit from JDK8
On 09/09/2013 04:28, David Holmes wrote: : Also: test/java/nio/channels/spi/SelectorProvider/inheritedChannel/lib/solaris-amd64/libLauncher.so I know this already exist but I thought binaries were disallowed in the open repo? Right, we don't want binary files checked in. The test needs to exercise System.inheritedChannel and so requires launching the VM with stdin/stdout/stderr connected to various types of networking sockets (to emulate launching from inetd/xnetd). That's an awkward scenario to setup so the test uses a custom launcher. We originally checked it in because we couldn't assume there would be compilers installed on all machines running the test. There's a bug open (JDK-6930061) to look at alternatives. It's not blocker to Kumar's changes of course. -Alan
Re: RFR: Changes to disable and/or remove Solaris 32-bit from JDK8
On 2013-09-06 18:47, Kumar Srinivasan wrote: Hello, Please review the changes to remove Solaris 32-bit binaries from JDK8 distros, at this time the dual mode support in the launcher is being disabled. Message regarding this: http://mail.openjdk.java.net/pipermail/jdk8-dev/2013-September/003159.html The jdk changes are here: http://cr.openjdk.java.net/~ksrini/8020552/webrev.jdk.0/ The top forest changes are here: http://cr.openjdk.java.net/~ksrini/8020552/webrev.jdk8.0/ In Jprt.gmk, the if and else clauses at the test for solaris-64 are identical, so you can remove the whole test and just keep the SRC_J*_IMAGE_DIR assignment. Apart from that, the build system changes looks fine. /Magnus
hg: jdk8/tl/jdk: 8023168: Cleanup LogManager class initialization and LogManager/LoggerContext relationship; ...
Changeset: 4afdf10de648 Author:dfuchs Date: 2013-09-09 13:59 +0200 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/4afdf10de648 8023168: Cleanup LogManager class initialization and LogManager/LoggerContext relationship 8021003: java/util/logging/Logger/getGlobal/TestGetGlobalConcurrent.java fails intermittently 8019945: test/java/util/logging/LogManagerInstanceTest.java failing intermittently Summary: This fix untangles the class initialization of Logger and LogManager, and also cleans up the relationship between LogManager, LoggerContext, and Logger, which were at the root cause of some intermittent test failures. Reviewed-by: mchung, martin, plevart ! src/share/classes/java/util/logging/LogManager.java ! src/share/classes/java/util/logging/Logger.java ! test/java/util/logging/Logger/getGlobal/TestGetGlobal.java ! test/java/util/logging/Logger/getGlobal/TestGetGlobalConcurrent.java ! test/java/util/logging/Logger/getGlobal/policy ! test/java/util/logging/ParentLoggersTest.java ! test/java/util/logging/TestAppletLoggerContext.java
hg: jdk8/tl/jdk: 2 new changesets
Changeset: 02064634bc88 Author:msheppar Date: 2013-09-06 15:00 +0100 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/02064634bc88 8023326: [TESTBUG] java/net/CookieHandler/LocalHostCookie.java misplaced try/finally Summary: amended test to be more robust to set of potential exceptions thrown Reviewed-by: chegar, khazra ! test/java/net/CookieHandler/LocalHostCookie.java Changeset: 4fd7abaf0a5f Author:msheppar Date: 2013-09-09 13:44 +0100 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/4fd7abaf0a5f 8021372: NetworkInterface.getNetworkInterfaces() returns duplicate hardware address Summary: amended src/windows/native/java/net/NetworkInterface_winXP.c to "properly" handle Ipv6IfIndex Reviewed-by: chegar, dsamersoff ! src/windows/native/java/net/NetworkInterface_winXP.c + test/java/net/NetworkInterface/UniqueMacAddressesTest.java
Re: Please review two corrections for java.time
Hi David, I found even on my VirturalBox machine it frequently came close to the .1ms target and failed in one case. Raising the time was to reduce/prevent intermittent failures. Are other timing tests also sensitive to the Xcomp, how should tests be written to be insensitive to that JVM option? Are you otherwise ok with the changes? Thanks, Roger On 9/8/2013 10:43 PM, David Holmes wrote: Hi Roger, On 7/09/2013 3:58 AM, roger riggs wrote: Please review for two corrections: - The java/time/tck/java/time/TCKLocalTime test failed on a slow machine; the test should be more lenient. The test is not appropriate for a conformance test and is moved to java/time/test/java/time/TestLocalTime. As per the bug report the issue is not slow machines per-se but the use of Xcomp when running the test. I don't think the jck should ever be run with Xcomp. It will be interesting to see if the change from 100ms to 500ms cures the problem on all machines. Thanks, David - The javadoc for the JapaneseEra.MEIJI era should indicate the start date is 1868-01-01 to be consistent with java.util.Calendar. Note that java.time does not permit dates before Meiji 6 to be created since the calendar is not clearly defined until then. Webrev: http://cr.openjdk.java.net/~rriggs/webrev-localtime-now-8023639/ Thanks, Roger
Re: Please review two corrections for java.time
Hi Peter,
The possible wrap-around caused by crossing midnight is handled by Math.max
so a retry is not needed.
Math.abs(test.toNanoOfDay() - expected.toNanoOfDay())
Thanks, Roger
On 9/9/2013 2:14 AM, Peter Levart wrote:
On 09/06/2013 07:58 PM, roger riggs wrote:
Please review for two corrections:
- The java/time/tck/java/time/TCKLocalTime test failed on a slow
machine;
the test should be more lenient. The test is not appropriate
for a conformance test
and is moved to java/time/test/java/time/TestLocalTime.
- The javadoc for the JapaneseEra.MEIJI era should indicate the start
date is 1868-01-01
to be consistent with java.util.Calendar. Note that java.time does
not permit dates before Meiji 6
to be created since the calendar is not clearly defined until then.
Webrev: http://cr.openjdk.java.net/~rriggs/webrev-localtime-now-8023639/
Thanks, Roger
Hi Roger,
Although very in-probable, the test can fail when 'expected' is
sampled before and 'test' is sampled after midnight. I'm guessing the
test is trying to prove that LocalTime.now() is equivalent to
LocalTime.now(Clock.systemDefaultZone()), right?
In that case, what about the following:
public void now() {
LocalTime before, test, after;
do {
before = LocalTime.now(Clock.systemDefaultZone());
test = LocalTime.now();
after = LocalTime.now(Clock.systemDefaultZone());
// retry in case the samples were obtained around midnight
} while (before.compareTo(after) > 0);
assertTrue(before.compareTo(test) <= 0 &&
test.compareTo(after) <= 0);
}
Regards, Peter
Re: RFR 8010293 Re: Potential issue with CHM.toArray
On Sep 6, 2013, at 4:56 PM, Alan Bateman wrote:
> On 06/09/2013 12:08, Paul Sandoz wrote:
>> On Sep 2, 2013, at 3:24 PM, Paul Sandoz wrote:
>>
>> :
>>
>>> Here is the fix in the lambda repo which can be applied to tl:
>>>
>>> http://hg.openjdk.java.net/lambda/lambda/jdk/rev/b73937e96ae0
>>> http://hg.openjdk.java.net/lambda/lambda/jdk/raw-rev/b73937e96ae0
>>>
>> Anyone up for reviewing this?
>>
> The comments are very educational as the resizing is difficult to completely
> grok without going through examples on a whiteboard. Anyway, I don't see
> anything obviously wrong after going through it. The test case is useful
> although creating the list of threads is quite a mouth full to take in.
>
Yeah, i left that in a convoluted intermediate state and wanted to use
CountedCompleter instead, see below for a revised and preferred version.
Paul.
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.ConcurrentHashMap;
import java.util.stream.IntStream;
public class toArray {
public static void main(String[] args) throws Throwable {
// Execute a number of times to increase the probability of
// failure if there is an issue
for (int i = 0; i < 16; i++) {
executeTest();
}
}
static void executeTest() throws Throwable {
final Throwable throwable[] = new Throwable[1];
final ConcurrentHashMap m = new ConcurrentHashMap<>();
// Number of workers equal to the number of processors
final int nWorkers = Runtime.getRuntime().availableProcessors();
final int sizePerWorker = 1024;
final int maxSize = nWorkers * sizePerWorker;
// The foreman keeps checking that the size of the arrays
// obtained from the key and value sets is never less than the
// previously observed size and is never greater than the maximum size
// NOTE: these size constraints are not specific to toArray and are
// applicable to any form of traversal of the collection views
CompletableFuture foreman = CompletableFuture.runAsync(new
Runnable() {
private int prevSize = 0;
private boolean checkProgress(Object[] a) {
int size = a.length;
if (size < prevSize) throw new RuntimeException("WRONG WAY");
if (size > maxSize) throw new RuntimeException("OVERSHOOT");
if (size == maxSize) return true;
prevSize = size;
return false;
}
@Override
public void run() {
try {
Integer[] empty = new Integer[0];
while (true) {
if (checkProgress(m.values().toArray())) return;
if (checkProgress(m.keySet().toArray())) return;
if (checkProgress(m.values().toArray(empty))) return;
if (checkProgress(m.keySet().toArray(empty))) return;
}
}
catch (Throwable t) {
throwable[0] = t;
}
}
});
// Create workers
// Each worker will put globally unique keys into the map
CompletableFuture[] workers = IntStream.range(0, nWorkers).
mapToObj(w -> CompletableFuture.runAsync(() -> {
for (int i = 0, o = w * sizePerWorker; i < sizePerWorker;
i++)
m.put(o + i, i);
})).
toArray(CompletableFuture[]::new);
// Wait for workers and then foreman to complete
CompletableFuture.allOf(workers).join();
foreman.join();
if (throwable[0] != null)
throw throwable[0];
}
}
Re: RFR(S) / guidance, 8022701 Accessibility checking: InvocationTargetException is thrown instead of IllegalAccessError
On 2013-09-08, at 10:39 PM, David Holmes wrote: > On 7/09/2013 1:28 AM, Alan Bateman wrote: >> On 06/09/2013 15:18, David Chase wrote: >>> webrev: http://cr.openjdk.java.net/~drchase/8022701/webrev.00/ >>> >>> Question #2, what's the best way to write a jtreg test suite that >>> requires incompatible class files, that could not result from a single >>> javac compilation? >> Can you coerce ASM into creating it? Alternatively it is something that >> you can create off-line and include in a class as a byte array (to load >> via your own URLClassLoader)? > > Multiple @compile tags in the main test? I did it with a hacked classloader instead, see the message with RFR(S+M) / 8022701 Accessibility checking: InvocationTargetException is thrown instead of IllegalAccessError (webrev: http://cr.openjdk.java.net/~drchase/8022701/webrev.02/ ) I figured it could not be "small" if it included multiple files in the test. David
Re: RFR JDK-8011940 : java.lang.Class.getAnnotations() always enters synchronized method
Hi Peter,
Thanks for this, please add a "@bug 8011940" tag to your test. IMO you don't
need a re-review for that. Otherwise looks good.
We still need a Reviewer, Chris, you reviewed a previous version, can you look
at this one too?
cheers
/Joel
On 27 aug 2013, at 15:00, Peter Levart wrote:
> Hi Joel and others,
>
> Here's a 3rd revision of this proposed patch:
>
> http://cr.openjdk.java.net/~plevart/jdk8-tl/AnnotationData/webrev.03/
>
> I used LinkedHashMap for annotations in this one. It means that now even
> .getAnnotations() are reported in "declaration order": 1st inherited
> (includes overridden) then declared (that are not overriding). For example,
> using @Inherited annotations A1, A2, A3:
>
> @A1("C")
> @A2("C")
> class C {}
>
> @A1("D")
> @A3("D")
> class D extends C {}
>
> D.class.getAnnotations() now returns: { @A1("D"), @A2("C"), @A3("D") } ...
>
> The LHM constructor uses the following expression to estimate the initial
> capacity of the LHM:
>
> 3326 annotations = new LinkedHashMap<>((Math.max(
> 3327 declaredAnnotations.size(),
> 3328 Math.min(12, declaredAnnotations.size()
> + superAnnotations.size())
> 3329 ) * 4 + 2) / 3
> 3330 );
>
>
> I think this strikes some balance between effectivity and accuracy of
> estimation. I could pre-scan the superclass annotations and calculate the
> exact final size of the annotations Map before constructing it though. Tell
> me if this is worth the effort.
>
> I also created a test that tests 3 things:
> - class annotation inheritance
> - order of class annotations reported by .getAnnotations() and
> .getDeclaredAnnotations() methods (although not specified, the order is now
> stable and resembles declaration order)
> - behaviour of class annotation caching when class is redefined
>
>
> Regards, Peter
>
> On 08/13/2013 09:52 AM, Joel Borggrén-Franck wrote:
>> Hi,
>>
>> Comments inline,
>>
>> On 12 aug 2013, at 14:40, Peter Levart wrote:
>>> On 08/12/2013 12:54 PM, Joel Borggren-Franck wrote:
>>> - annotation (@interface) declarations can themselves be redefined (for
>>> example, defaults changed). Such redefinitions don't affect already
>>> initialized annotations. Default values are cached in AnnotationType which
>>> is not invalidated as a result of class redefinition. Maybe it should be.
>>> And even if AnnotationType was invalidated as a result of class
>>> redefinition, the defaults are looked-up when particular annotations are
>>> initialized and then combined with parsed values in a single values map
>>> inside each annotation instance (proxy), so invalidating AnnotationType
>>> would have no effect on those annotations.
>>>
3326 if (annotations == null) { // lazy construction
3327 annotations = new HashMap<>();
3328 }
I think this should be a LinkedHashMap, so that iteration is predictable
(I know it isn't in the current code). Also the size of the map is known
so you can use a constructor with an explicit initial capacity.
>>> Right, AnnotationParser does return LinkedHashMap, so at least
>>> decalredAnnotations are in parse-order. I will change the code to use
>>> LinkedHashMap for inherited/combined case too.
>> Thanks.
>>
>>> The number of annotations that end-up in inherited/combined Map is not
>>> known in advance. I could make a separate pre-scan for superclass
>>> annotations that are @Inheritable and don't have the same key as any of
>>> declared annotations and then sum this count with declared annotations
>>> count, but I don't think this will be very effective. I could allocate a
>>> large-enough Map to always fit (the count of superclass annotations + the
>>> count of declared annotations), but that could sometimes lead to much
>>> over-allocated Maps. I could take the min(DEFAULT_CAPACITY, superclass
>>> annotations count + declared annotations count) as the initial capacity for
>>> the Map. What do you think which of those 3 alternatives is the best?
>> My bad. I was thinking of the case where every inherited annotation was
>> overridden, in that case annotations.size() == declaredAnnotations.size().
>> That is of course not generally true. I'm fine with handling this as a
>> follow up since the situation is no worse than today and the surrounding
>> code is better. However,
>>
>> 1) We should really measure this.
>> 2) My gut feeling is that the ratio of not overridden inherited annotations
>> to declared annotations is small IE the expected nr of annotations is much
>> closer to declare annotations than to declared + superclass annotations.
>> 3) The default initial capacity is 16 and load factor is 0.75. I do think
>> the mean nr of annotations is closer to 3 than to 12. We are probably
>> wasting some space here.
>>
>> Perhaps use min(def
Re: RFR: 8023447: change specification to allow RMI activation to be optional
Stuart, Thanks for clarifying. I agree with minimization effort and your statements below. So the this looks all fine ! Olivier Stuart Marks said on date 9/7/2013 2:31 AM: Hi Olivier, Thanks for looking at this. Part of the minimization effort (see my reply to Alan) showed that ActivationGroupDesc needn't have the throws-UOE specs added to it. This is pretty easy to see by going to the "Use" tab of the ActivationGroupDesc javadoc. ActivationGroupDesc instances are returned and consumed by several instance methods of ActivationSystem; this OK since we prevent the application from ever getting any ActivationSystem instances, by specifying UOE wherever they're returned. ActivationGroupDesc is also consumed by a static method ActivationGroup.createGroup(). That has UOE on it, so we're covered. The intuition behind this makes sense. If you look at ActivationGroupDesc, it's just a "data class" -- it merely contains Strings, MarshalledObjects, and Properties. Well, it has a CommandEnvironment, but that's just a data class too. As such, ActivationGroupDesc is just a bag of data that's used by the other activation classes. (Note also that it doesn't throw any activation exceptions anywhere.) Anyway, there's no harm in allowing apps to create ActivationGroupDesc instances, so we didn't add UOE there. I tried similar analysis on some of the other objects and was unsuccessful at convincing myself they didn't need UOEs added to them, so that's how we ended up with so many cases where throwing UOE is necessary. s'marks On 9/6/13 2:38 AM, Olivier Lagneau wrote: Hi Stuart, I like this, and think this is the right approach for solving the problem. I see however that ActivationGroupDesc is not impacted by the change. In the case of an implementation that does not support activation, are we sure there will be no way for a developer to create an instance of any the other key activation classes (ActivationGroup, ActivationDesc, ActivationGroupId, ActivationID), using an "Activatable" object (one that does not subclass Activatable) and an ActivationGroupDesc instance, through any "default" behaviour described in the spec ? If that has been checked, that looks fine. Olivier. Stuart Marks said on date 9/6/2013 12:46 AM: Hi all, Please review this specification-only change to allow RMI activation to be optional. RMI activation, unlike the rest of RMI, pretty much requires the ability to fork processes at will. This causes difficulties in certain situations, such as in small embedded configurations. Activation is typically unnecessary in such environments, hence it makes sense for it to be optional. Essentially the change is the addition of a small paragraph to the package doc for java.rmi.activation, and adding spec for throwing UnsupportedOperationException to a bunch of methods in this package. Bug report: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8023447 Webrev: http://cr.openjdk.java.net/~smarks/reviews/8023447/webrev.5/ Specdiff: http://cr.openjdk.java.net/~smarks/reviews/8023447/specdiff.5/overview-summary.html Thanks, s'marks
Re: Please review two corrections for java.time
On 09/09/2013 03:12 PM, roger riggs wrote:
Hi Peter,
The possible wrap-around caused by crossing midnight is handled by
Math.max
so a retry is not needed.
Math.abs(test.toNanoOfDay() - expected.toNanoOfDay())
Hi Roger,
In case there is a wrap-around, the 'diff' is much more than 500,000,000
ns (about 24*60*60*1,000,000,000 ns - delay), which fails the test.
But what do you think about testing before <= test <= after ? It should
not be timing dependent, like it is now. Does it test the same thing?
Regards, Peter
Thanks, Roger
On 9/9/2013 2:14 AM, Peter Levart wrote:
On 09/06/2013 07:58 PM, roger riggs wrote:
Please review for two corrections:
- The java/time/tck/java/time/TCKLocalTime test failed on a slow
machine;
the test should be more lenient. The test is not appropriate
for a conformance test
and is moved to java/time/test/java/time/TestLocalTime.
- The javadoc for the JapaneseEra.MEIJI era should indicate the
start date is 1868-01-01
to be consistent with java.util.Calendar. Note that java.time
does not permit dates before Meiji 6
to be created since the calendar is not clearly defined until then.
Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-localtime-now-8023639/
Thanks, Roger
Hi Roger,
Although very in-probable, the test can fail when 'expected' is
sampled before and 'test' is sampled after midnight. I'm guessing the
test is trying to prove that LocalTime.now() is equivalent to
LocalTime.now(Clock.systemDefaultZone()), right?
In that case, what about the following:
public void now() {
LocalTime before, test, after;
do {
before = LocalTime.now(Clock.systemDefaultZone());
test = LocalTime.now();
after = LocalTime.now(Clock.systemDefaultZone());
// retry in case the samples were obtained around midnight
} while (before.compareTo(after) > 0);
assertTrue(before.compareTo(test) <= 0 &&
test.compareTo(after) <= 0);
}
Regards, Peter
Re: RFR: Changes to disable and/or remove Solaris 32-bit from JDK8
Hi David,
Hi Kumar,
This is still dead code in
src/share/classes/com/sun/tools/jdi/SunCommandLineLauncher.java
String os_arch = System.getProperty("os.arch");
Ah, I will take care of it. Thanks for spotting this.
Also:
test/java/nio/channels/spi/SelectorProvider/inheritedChannel/lib/solaris-amd64/libLauncher.so
I know this already exist but I thought binaries were disallowed in
the open repo?
Alan, are the nio changes acceptable? Let me know if you need more time
to go over all
the changes.
Kumar
Davud
On 9/09/2013 1:09 PM, Kumar Srinivasan wrote:
Hi David, Staffan, Alan,
I have addressed all the issues pointed and some more I found while jprt
testing.
The updated webrev for jdk is here:
http://cr.openjdk.java.net/~ksrini/8020552/webrev.jdk.1/
and the delta webrev since the last review webrev is here:
http://cr.openjdk.java.net/~ksrini/8020552/webrev.jdk.1/webrev.delta/index.html
Thanks
Kumar
Hi Kumar,
A few minor comments ...
src/share/classes/com/sun/tools/jdi/SunCommandLineLauncher.java
Seems to me this is all dead now:
199 /*
200 * A wrinkle in the environment:
201 * 64-bit executables are stored under
$JAVA_HOME/bin/os_arch
202 * 32-bit executables are stored under
$JAVA_HOME/bin
203 */
204 String os_arch = System.getProperty("os.arch");
os_arch is no longer used and the comment no longer applicable.
---
src/solaris/bin/java_md_solinux.c
This seems to force DUAL_MODE off regardless of what the user may set
it to:
#ifdef __solaris__
! # ifdef DUAL_MODE
! #undef DUAL_MODE
! # endif
why doesn't it just not define DUAL_MODE?
---
test/demo/jvmti/DemoRun.java
test/sun/tools/jhat/HatRun.java
It isn't clear to me why you need to retain the d64 variable at all.
---
test/tools/launcher/ExecutionEnvironment.java
typo: appopriate
Thanks,
David
On 7/09/2013 2:47 AM, Kumar Srinivasan wrote:
Hello,
Please review the changes to remove Solaris 32-bit binaries from JDK8
distros,
at this time the dual mode support in the launcher is being disabled.
Message regarding this:
http://mail.openjdk.java.net/pipermail/jdk8-dev/2013-September/003159.html
The jdk changes are here:
http://cr.openjdk.java.net/~ksrini/8020552/webrev.jdk.0/
The top forest changes are here:
http://cr.openjdk.java.net/~ksrini/8020552/webrev.jdk8.0/
Thanks
Kumar
Re: RFR: Changes to disable and/or remove Solaris 32-bit from JDK8
On 09/09/2013 16:12, Kumar Srinivasan wrote: Alan, are the nio changes acceptable? Let me know if you need more time to go over all the changes. It looks fine (sorry I should have made that clearer). I skimmed over the other tests too (the launcher tests in particular) and don't see any other issues. -Alan.
Re: RFR 8010293 Re: Potential issue with CHM.toArray
On Sep 9, 2013, at 3:35 PM, Paul Sandoz wrote: > > On Sep 6, 2013, at 4:56 PM, Alan Bateman wrote: > >> On 06/09/2013 12:08, Paul Sandoz wrote: >>> On Sep 2, 2013, at 3:24 PM, Paul Sandoz wrote: >>> >>> : >>> Here is the fix in the lambda repo which can be applied to tl: http://hg.openjdk.java.net/lambda/lambda/jdk/rev/b73937e96ae0 http://hg.openjdk.java.net/lambda/lambda/jdk/raw-rev/b73937e96ae0 >>> Anyone up for reviewing this? >>> >> The comments are very educational as the resizing is difficult to completely >> grok without going through examples on a whiteboard. Anyway, I don't see >> anything obviously wrong after going through it. The test case is useful >> although creating the list of threads is quite a mouth full to take in. >> > > Yeah, i left that in a convoluted intermediate state and wanted to use > CountedCompleter instead, see below for a revised and preferred version. > s/CountedCompleter/CompletableFuture Sigh F/J on the brain... Paul.
Re: Please review two corrections for java.time
On 09/09/2013 05:14 PM, Peter Levart wrote:
On 09/09/2013 03:12 PM, roger riggs wrote:
Hi Peter,
The possible wrap-around caused by crossing midnight is handled by
Math.max
so a retry is not needed.
Math.abs(test.toNanoOfDay() - expected.toNanoOfDay())
Hi Roger,
In case there is a wrap-around, the 'diff' is much more than
500,000,000 ns (about 24*60*60*1,000,000,000 ns - delay), which fails
the test.
But what do you think about testing before <= test <= after ? It
should not be timing dependent, like it is now. Does it test the same
thing?
Regards, Peter
One other possibility (admittedly even less probable) is when LocalTime
jumps back (when Daylight Savings Time rules instruct so). Re-trying
and making sure before/after samples are in increasing order also
catches this situation.
Regards, Peter
Thanks, Roger
On 9/9/2013 2:14 AM, Peter Levart wrote:
On 09/06/2013 07:58 PM, roger riggs wrote:
Please review for two corrections:
- The java/time/tck/java/time/TCKLocalTime test failed on a slow
machine;
the test should be more lenient. The test is not appropriate
for a conformance test
and is moved to java/time/test/java/time/TestLocalTime.
- The javadoc for the JapaneseEra.MEIJI era should indicate the
start date is 1868-01-01
to be consistent with java.util.Calendar. Note that java.time
does not permit dates before Meiji 6
to be created since the calendar is not clearly defined until then.
Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-localtime-now-8023639/
Thanks, Roger
Hi Roger,
Although very in-probable, the test can fail when 'expected' is
sampled before and 'test' is sampled after midnight. I'm guessing
the test is trying to prove that LocalTime.now() is equivalent to
LocalTime.now(Clock.systemDefaultZone()), right?
In that case, what about the following:
public void now() {
LocalTime before, test, after;
do {
before = LocalTime.now(Clock.systemDefaultZone());
test = LocalTime.now();
after = LocalTime.now(Clock.systemDefaultZone());
// retry in case the samples were obtained around midnight
} while (before.compareTo(after) > 0);
assertTrue(before.compareTo(test) <= 0 &&
test.compareTo(after) <= 0);
}
Regards, Peter
Re: [PATCH] 4851444: Exposing sun.reflect.Reflection#getCallerClass as a public API in Java 8
Nick,
Do you have any information to some of the questions I asked below that
can help the API discussion?
We need to decide on the permission check and also the hotspot change
has to be integrated and the jdk change will need to wait until it shows
in a promoted build. Schedule-wise, to make JDK 8, we should finalize
the API this week so that you can update the patch soon.
Mandy
On 9/3/2013 5:02 PM, Mandy Chung wrote:
Nick,
I skimmed through the changes. Congratulations for your first patch
making changes in both hotspot and jdk code.
In my mind, the Log4J use case accessing Class instance to obtain
additional information for diagnosability is different than the use
case of obtaining the caller's class / loader to lookup resources.
While the new APIs might be in the same class, I will discuss them
individually and keep us focus one at a time.
Ralph has pointed out [1] that Log4j also needs the ability to find an
appropriate ClassLoader which is used for logging separation (thank
you Ralph). That will be the caller-sensitivity (i.e. obtain caller's
class/loader) discussion.
There are a couple of RFEs:
https://bugs.openjdk.java.net/browse/JDK-4942697
https://bugs.openjdk.java.net/browse/JDK-6349551
Performance is critical for Log4j to traverse the stack trace and
obtain Class information. I like your patch to speed up the
generation of StackTraceElement[] (and StackTraceFrame[] - essentially
same code with different type). java.util.logging.LogRecord has
workaround the performance overhead and go to a specific frame
directly and avoid the cost of generating the entire array.
JDK-6349551 requests to lazily instantiate the StackTraceElement
object unless that frame is requested. Does Log4J always walk all
frames and log all information? Do you just log only some max number
of frames rather than the entire stack trace?
Class getDeclaringClass() method is the key method you need to
enhance the diagnosability. This method opens up a new way to access
a Class instance that untrusted code wouldn't be able in the past. A
permission check is needed as Alan points out early. Performance as
well as logging framework can't access all class loaders are two
factors to be considered when defining the permission check.
I saw your other comment about permission check (cut-n-paste below).
It seems to me that you don't agree a permission check is needed for
the getDeclaringClass() method (regardless of which class it belongs
to) and if that's the case, no point to continue.
I want to make it very clear that I have agreed to take this on and
provide a replacement of sun.reflect.Reflection.getCallerClass(int) in
JDK 8 to address the use cases. It will take time for the API and
security discussion and please be prepared for that (also I am working
on other things at the same time).
The second comment on your patch is that there are lot of duplicated
code in hotspot in order to support two similar but different types
(StackTraceFrame and StackTraceElement). Serialization is the reason
leading you to have a new StackTraceFrame class. Maybe some
refactoring can help but this is the cost of having the VM directly
creating the instance. One other option, as suggested in the previous
thread, is to keep the declaring class in the StackTraceElement as a
transient field. If we add the getDeclaringClass method in the
StackTraceElement class, it would need to specify to be optional that
it only returns the Class instance when it's available.
There are currently three different ways to get a stack trace:
1. Throwable.getStackTrace()
2. Thread.getStackTrace() and Thread.getAllStackTraces()
3. java.lang.management.ThreadMXBean.getThreadInfo(long id, int
maxDepth).getStackTrace() and multiple thread IDs version
(a) local (b) remote
Since it's a new StackTraceFrame class, you have to provide a new
method replacing #1 and #2. I don't see any need to provide a new API
equivalent to Thread.getAllStackTraces() and java.lang.management.
The proposal I originally have last week was to keep declaring class
as transient and add a method in StackTraceElement with a permission
check at every call. Tom raises a good question about the cost of
permission check. Would that be a concern to Log4j? Is log4j on
bootclasspath or extension directory? I assume not. So for log4j to
work with security manager installed, it would have torequire the
application to grant certain permissions - can you confirm? For
example it calls sun.reflect.Reflection.getCallerClass method that
will require RuntimePermission("accessClassInPackage.sun.reflect")
permission. Calling Class.getProtectionDomain and
Class.getClassLoader() requires
RuntimePermission("getProtectionDomain") and
RuntimePermission("getClassLoader") respectively. That gives me an
impression that permission check on individual stack frame might be a
non-issue?
Mandy
[1]
http://mail.openjdk.java.net/piperm
hg: jdk8/tl/langtools: 8024154: Fix for 8016177: structural most specific and stuckness breaks 6 langtools tests
Changeset: a4b9a8859e58 Author:vromero Date: 2013-09-09 16:32 +0100 URL: http://hg.openjdk.java.net/jdk8/tl/langtools/rev/a4b9a8859e58 8024154: Fix for 8016177: structural most specific and stuckness breaks 6 langtools tests Reviewed-by: jjg, jfranck ! test/tools/javac/lambda/MethodReference41.java ! test/tools/javac/lambda/MethodReference41.out ! test/tools/javac/lambda/MethodReference42.java ! test/tools/javac/lambda/MethodReference42.out ! test/tools/javac/lambda/MethodReference43.java ! test/tools/javac/lambda/MethodReference43.out ! test/tools/javac/lambda/MethodReference44.java ! test/tools/javac/lambda/MethodReference44.out ! test/tools/javac/lambda/MethodReference46.java ! test/tools/javac/lambda/MethodReference46.out ! test/tools/javac/lambda/MethodReference48.java ! test/tools/javac/lambda/MethodReference48.out
Re: RFR: 8009411 : getMethods should not inherit static methods from interfaces
Hi Joel,
The fix is ok for getMethods(), but I think the getMethod(String name,
Class... parameterTypes) should also be fixed. Currently the
following code:
public class StaticInterfaceMethodTest {
public interface A {
static void m() {}
}
public interface B extends A {
}
public static void main(String[] args) throws Exception {
B.class.getMethod("m");
}
}
...succeeds, but should throw NoSuchMethodException, I think.
Regards, Peter
On 09/09/2013 07:00 PM, Joel Borggrén-Franck wrote:
Hi
Pleaser review fix for 8009411 : getMethods should not inherit static methods
from interfaces
The issue is that since we added static methods to interfaces those have
erroneously been reflected in getMethods of implementing classes. This fix
filters out static interface methods from superinterfaces when adding methods.
I have also added a note to the javadoc for both getMembers and
getDeclaredMembers pointing this out (though it is implied from JLS). Webrev is
based on the clarification to getMethods and friends out for review on this
list.
Webrev: http://cr.openjdk.java.net/~jfranck/8009411/webrev.00/
Bug is here: http://bugs.sun.com/view_bug.do?bug_id=8009411
For oracle reviewers, ccc is approved.
cheers
/Joel
Re: [PATCH] 4851444: Exposing sun.reflect.Reflection#getCallerClass as a public API in Java 8
On 09/08/2013 09:34 PM, Mandy Chung wrote: On 9/4/2013 6:02 PM, David M. Lloyd wrote: This seems reasonable on the surface but falls over once you capture the caller for more than one purpose. For example, say a logging framework captures the caller for the purpose of supplementing log information. But you call this logging framework from another framework which uses caller information for another purpose, for example locating resources. The intent here might be to show information from the second framework in the log, however with one universal @CallerSensitive annotation you cannot distinguish which "capture" you want to grab - the second framework, or the caller who called the second framework. However by traversing the stack to a fixed depth, you can do so very definitively (as long as you always know that your internal code does *not* directly call the sensitive method - an easy thing to design for in most frameworks). It would need to detect if the intermediate frames don't call any "sensitive" method. @sun.reflect.CallerSensitive is primarily defined for the security issue and specifically for JEP 176. As you said, the current form of @CS doesn't satisfy other purposes. In fact you can usually traverse the stack to a fixed depth for this kind of thing, with one key exception that comes up in log frameworks. When you have one log API which forwards to another, you want to capture the "first" caller of any log API. Pursuant to this, most log frameworks have log method variants which accept the fully-qualified class name of that first logger. The moral equivalent to this scenario would likely be to provide an API variant which accepts a Class or ClassLoader (depending on the usage) and a variant which does not and uses a fixed-depth "reach" into the stack instead. This IMO blows a hole in the whole idea of a single *public* @CS annotation, and in fact in public framework code, a depth indicator seems to be adequate and more or less problem-free for any purpose I've run across. I'm not sure if we can be very certain about the depth in a runtime environment (non-debugging) unless it requires all VM implementation to support a reliable way to return a frame at a given depth. The stack trace is not guaranteed to contain all stack frames. E.g. in the spec of Throwable.getStackTrace(): Some virtual machines may, under some circumstances, omit one or more stack frames from the stack trace. In the extreme case, a virtual machine that has no stack trace information concerning this throwable is permitted to return a zero-length array from this method. This is interesting. Presumably this would tie into tail-call optimizations and that sort of thing? How does AccessControlContext deal with this possibility? -- - DML
RFR: 8009411 : getMethods should not inherit static methods from interfaces
Hi Pleaser review fix for 8009411 : getMethods should not inherit static methods from interfaces The issue is that since we added static methods to interfaces those have erroneously been reflected in getMethods of implementing classes. This fix filters out static interface methods from superinterfaces when adding methods. I have also added a note to the javadoc for both getMembers and getDeclaredMembers pointing this out (though it is implied from JLS). Webrev is based on the clarification to getMethods and friends out for review on this list. Webrev: http://cr.openjdk.java.net/~jfranck/8009411/webrev.00/ Bug is here: http://bugs.sun.com/view_bug.do?bug_id=8009411 For oracle reviewers, ccc is approved. cheers /Joel
Re: RFR: 8009411 : getMethods should not inherit static methods from interfaces
Hi Peter,
You are correct, thanks for noticing this. Also after reading your mail I
looked through the test in test/java/lang/refledt/DefaultStaticTest/ and just
realized the tests doesn't cover getMethod() at all. So this change needs more
tests as well.
I'll post a new webrev later this week.
cheers
/Joel
On 9 sep 2013, at 19:24, Peter Levart wrote:
> Hi Joel,
>
> The fix is ok for getMethods(), but I think the getMethod(String name,
> Class... parameterTypes) should also be fixed. Currently the following
> code:
>
> public class StaticInterfaceMethodTest {
> public interface A {
> static void m() {}
> }
>
> public interface B extends A {
> }
>
> public static void main(String[] args) throws Exception {
> B.class.getMethod("m");
> }
> }
>
>
> ...succeeds, but should throw NoSuchMethodException, I think.
>
>
> Regards, Peter
>
> On 09/09/2013 07:00 PM, Joel Borggrén-Franck wrote:
>> Hi
>>
>> Pleaser review fix for 8009411 : getMethods should not inherit static
>> methods from interfaces
>>
>> The issue is that since we added static methods to interfaces those have
>> erroneously been reflected in getMethods of implementing classes. This fix
>> filters out static interface methods from superinterfaces when adding
>> methods. I have also added a note to the javadoc for both getMembers and
>> getDeclaredMembers pointing this out (though it is implied from JLS). Webrev
>> is based on the clarification to getMethods and friends out for review on
>> this list.
>>
>> Webrev:
>> http://cr.openjdk.java.net/~jfranck/8009411/webrev.00/
>>
>> Bug is here:
>> http://bugs.sun.com/view_bug.do?bug_id=8009411
>>
>>
>> For oracle reviewers, ccc is approved.
>>
>> cheers
>> /Joel
>>
>
Re: RFR JDK-8011940 : java.lang.Class.getAnnotations() always enters synchronized method
Hi Joel,
Thanks for reviewing.
On 09/09/2013 04:25 PM, Joel Borggrén-Franck wrote:
Hi Peter,
Thanks for this, please add a "@bug 8011940" tag to your test. IMO you don't
need a re-review for that. Otherwise looks good.
I'll do that. I just didn't know whether tagging with bug-ID is meant
for all tests that arise from fixing a particular bug or just those that
test the presence/fixture of the bug. The 8011940 bug is about
scalability and the test is not testing scalability (it has been
demonstrated by a micro-benchmark, but that is not included in the
test). The test is just covering the logic that has been re-factored and
has not had any tests before.
Regards, Peter
We still need a Reviewer, Chris, you reviewed a previous version, can you look
at this one too?
cheers
/Joel
On 27 aug 2013, at 15:00, Peter Levart wrote:
Hi Joel and others,
Here's a 3rd revision of this proposed patch:
http://cr.openjdk.java.net/~plevart/jdk8-tl/AnnotationData/webrev.03/
I used LinkedHashMap for annotations in this one. It means that now even
.getAnnotations() are reported in "declaration order": 1st inherited (includes
overridden) then declared (that are not overriding). For example, using @Inherited
annotations A1, A2, A3:
@A1("C")
@A2("C")
class C {}
@A1("D")
@A3("D")
class D extends C {}
D.class.getAnnotations() now returns: { @A1("D"), @A2("C"), @A3("D") } ...
The LHM constructor uses the following expression to estimate the initial
capacity of the LHM:
3326 annotations = new LinkedHashMap<>((Math.max(
3327 declaredAnnotations.size(),
3328 Math.min(12, declaredAnnotations.size() +
superAnnotations.size())
3329 ) * 4 + 2) / 3
3330 );
I think this strikes some balance between effectivity and accuracy of
estimation. I could pre-scan the superclass annotations and calculate the exact
final size of the annotations Map before constructing it though. Tell me if
this is worth the effort.
I also created a test that tests 3 things:
- class annotation inheritance
- order of class annotations reported by .getAnnotations() and
.getDeclaredAnnotations() methods (although not specified, the order is now
stable and resembles declaration order)
- behaviour of class annotation caching when class is redefined
Regards, Peter
On 08/13/2013 09:52 AM, Joel Borggrén-Franck wrote:
Hi,
Comments inline,
On 12 aug 2013, at 14:40, Peter Levart wrote:
On 08/12/2013 12:54 PM, Joel Borggren-Franck wrote:
- annotation (@interface) declarations can themselves be redefined (for
example, defaults changed). Such redefinitions don't affect already initialized
annotations. Default values are cached in AnnotationType which is not
invalidated as a result of class redefinition. Maybe it should be. And even if
AnnotationType was invalidated as a result of class redefinition, the defaults
are looked-up when particular annotations are initialized and then combined
with parsed values in a single values map inside each annotation instance
(proxy), so invalidating AnnotationType would have no effect on those
annotations.
3326 if (annotations == null) { // lazy construction
3327 annotations = new HashMap<>();
3328 }
I think this should be a LinkedHashMap, so that iteration is predictable
(I know it isn't in the current code). Also the size of the map is known
so you can use a constructor with an explicit initial capacity.
Right, AnnotationParser does return LinkedHashMap, so at least
decalredAnnotations are in parse-order. I will change the code to use
LinkedHashMap for inherited/combined case too.
Thanks.
The number of annotations that end-up in inherited/combined Map is not known in
advance. I could make a separate pre-scan for superclass annotations that are
@Inheritable and don't have the same key as any of declared annotations and
then sum this count with declared annotations count, but I don't think this
will be very effective. I could allocate a large-enough Map to always fit (the
count of superclass annotations + the count of declared annotations), but that
could sometimes lead to much over-allocated Maps. I could take the
min(DEFAULT_CAPACITY, superclass annotations count + declared annotations
count) as the initial capacity for the Map. What do you think which of those 3
alternatives is the best?
My bad. I was thinking of the case where every inherited annotation was
overridden, in that case annotations.size() == declaredAnnotations.size(). That
is of course not generally true. I'm fine with handling this as a follow up
since the situation is no worse than today and the surrounding code is better.
However,
1) We should really measure this.
2) My gut feeling is that the ratio of not overridden inherited annotations to
declared annotations is small IE the expected nr of annotations is mu
Re: [PATCH] 4851444: Exposing sun.reflect.Reflection#getCallerClass as a public API in Java 8
Take this lightly informed suggestion with a grain of salt, but why not, for
purposes of performance and security,
change the logging-specific getCallerClass methods so that their "class"
references are instead wrapped in some sort of proxy object that only forwards
certain operations quickly without a security check? For example, equals,
hashcode, and toString are probably not security-sensitive.
i.e.
class SafeClass {
private final Class clazz;
public SafeClass(Class clazz) { this.clazz = class; }
public String toString() { return clazz.toString(); }
public int hashCode() { return clazz.hashCode(); }
public boolean equals(Object o) { return clazz.equals(o); }
public Class maybeWeLetYouLookAtTheRealClass() { ... a bunch of security
checks ... }
}
If necessary, do the same for classloaders.
And them, no security checks needed, as long as the "safe" methods are enough
to get the job done.
David
On 2013-09-09, at 10:54 AM, Mandy Chung wrote:
> Nick,
>
> Do you have any information to some of the questions I asked below that can
> help the API discussion?
>
> We need to decide on the permission check and also the hotspot change has to
> be integrated and the jdk change will need to wait until it shows in a
> promoted build. Schedule-wise, to make JDK 8, we should finalize the API
> this week so that you can update the patch soon.
>
> Mandy
>
> On 9/3/2013 5:02 PM, Mandy Chung wrote:
>> Nick,
>>
>> I skimmed through the changes. Congratulations for your first patch making
>> changes in both hotspot and jdk code.
>>
>> In my mind, the Log4J use case accessing Class instance to obtain additional
>> information for diagnosability is different than the use case of obtaining
>> the caller's class / loader to lookup resources. While the new APIs might
>> be in the same class, I will discuss them individually and keep us focus one
>> at a time.
>>
>> Ralph has pointed out [1] that Log4j also needs the ability to find an
>> appropriate ClassLoader which is used for logging separation (thank you
>> Ralph). That will be the caller-sensitivity (i.e. obtain caller's
>> class/loader) discussion.
>>
>> There are a couple of RFEs:
>> https://bugs.openjdk.java.net/browse/JDK-4942697
>> https://bugs.openjdk.java.net/browse/JDK-6349551
>>
>> Performance is critical for Log4j to traverse the stack trace and obtain
>> Class information. I like your patch to speed up the generation of
>> StackTraceElement[] (and StackTraceFrame[] - essentially same code with
>> different type). java.util.logging.LogRecord has workaround the performance
>> overhead and go to a specific frame directly and avoid the cost of
>> generating the entire array. JDK-6349551 requests to lazily instantiate the
>> StackTraceElement object unless that frame is requested. Does Log4J always
>> walk all frames and log all information? Do you just log only some max
>> number of frames rather than the entire stack trace?
>>
>> Class getDeclaringClass() method is the key method you need to enhance
>> the diagnosability. This method opens up a new way to access a Class
>> instance that untrusted code wouldn't be able in the past. A permission
>> check is needed as Alan points out early. Performance as well as logging
>> framework can't access all class loaders are two factors to be considered
>> when defining the permission check.
>>
>> I saw your other comment about permission check (cut-n-paste below). It
>> seems to me that you don't agree a permission check is needed for the
>> getDeclaringClass() method (regardless of which class it belongs to) and if
>> that's the case, no point to continue.
>>
>> I want to make it very clear that I have agreed to take this on and provide
>> a replacement of sun.reflect.Reflection.getCallerClass(int) in JDK 8 to
>> address the use cases. It will take time for the API and security
>> discussion and please be prepared for that (also I am working on other
>> things at the same time).
>>
>> The second comment on your patch is that there are lot of duplicated code in
>> hotspot in order to support two similar but different types (StackTraceFrame
>> and StackTraceElement). Serialization is the reason leading you to have a
>> new StackTraceFrame class. Maybe some refactoring can help but this is the
>> cost of having the VM directly creating the instance. One other option, as
>> suggested in the previous thread, is to keep the declaring class in the
>> StackTraceElement as a transient field. If we add the getDeclaringClass
>> method in the StackTraceElement class, it would need to specify to be
>> optional that it only returns the Class instance when it's available.
>>
>> There are currently three different ways to get a stack trace:
>> 1. Throwable.getStackTrace()
>> 2. Thread.getStackTrace() and Thread.getAllStackTraces()
>> 3. java.lang.management.ThreadMXBean.getThreadInfo(long id, int
>> maxDepth).getStackTrace() and multiple thread IDs version
hg: jdk8/tl/nashorn: 3 new changesets
Changeset: 7ae169639485 Author:sundar Date: 2013-09-05 21:17 +0530 URL: http://hg.openjdk.java.net/jdk8/tl/nashorn/rev/7ae169639485 8024255: When a keyword is used as object property name, the property can not be deleted Reviewed-by: jlaskey, lagergren ! src/jdk/nashorn/internal/parser/Parser.java + test/script/basic/JDK-8024255.js Changeset: c3b6ce7b74bf Author:sundar Date: 2013-09-09 20:10 +0530 URL: http://hg.openjdk.java.net/jdk8/tl/nashorn/rev/c3b6ce7b74bf 8024180: Incorrect handling of expression and parent scope in 'with' statements Reviewed-by: jlaskey, hannesw ! src/jdk/nashorn/api/scripting/NashornScriptEngine.java ! src/jdk/nashorn/api/scripting/ScriptObjectMirror.java ! src/jdk/nashorn/internal/objects/Global.java ! src/jdk/nashorn/internal/runtime/Context.java ! src/jdk/nashorn/internal/runtime/GlobalObject.java ! src/jdk/nashorn/internal/runtime/NativeJavaPackage.java ! src/jdk/nashorn/internal/runtime/ScriptObject.java ! src/jdk/nashorn/internal/runtime/ScriptRuntime.java ! src/jdk/nashorn/internal/runtime/WithObject.java ! src/jdk/nashorn/internal/runtime/resources/Messages.properties + test/script/basic/8024180/global_var_delete.js + test/script/basic/8024180/global_var_delete.js.EXPECTED + test/script/basic/8024180/global_var_shadow.js + test/script/basic/8024180/global_var_shadow.js.EXPECTED + test/script/basic/8024180/scope_no_such_prop.js + test/script/basic/8024180/scope_no_such_prop.js.EXPECTED + test/script/basic/8024180/with_expr_prop_add.js + test/script/basic/8024180/with_expr_prop_add.js.EXPECTED + test/script/basic/8024180/with_expr_proto_prop_add.js + test/script/basic/8024180/with_expr_proto_prop_add.js.EXPECTED + test/script/basic/8024180/with_java_object.js + test/script/basic/8024180/with_java_object.js.EXPECTED ! test/src/jdk/nashorn/internal/runtime/ContextTest.java Changeset: 1eca380a221f Author:sundar Date: 2013-09-09 20:16 +0530 URL: http://hg.openjdk.java.net/jdk8/tl/nashorn/rev/1eca380a221f Merge
Re: [PATCH] 4851444: Exposing sun.reflect.Reflection#getCallerClass as a public API in Java 8
After some thought the below idea seems not good. Currently j.l.Class objects can be used to "transfer" privilige from code that can obtain them to code that can't (like MHs for example). So doing what I proposed would disable this ability. Regards, Peter Hi, This is a good idea. It got me thinking that there are a bunch of methods in j.l.Class that are not security-sensitive. So instead of proxy-ing those in a wrapper SafeClass, let's just identify "unsafe" methods 1st. If the security checks that are planned for obtaining j.l.Class instances from call-stack are transferred to those unsafe methods instead, then holding a reference to a j.l.Class instance becomes a security-nonissue. Just a quick example - presumably the "unsafe" methods of j.l.Class are: get(Declared)Method(s), get(Declared)Field(s) and get(Declared)Constructor(s), because they enable you to call/access public methods/fields/constructors of a class represented by j.l.Class object. If this class is from a restricted package (say sun.misc) then you could get access to restricted methods/fields/instances. Now if the security checks for obtaining reflective object would include checking the "class visibility/restrictability", then j.l.Class object of say sun.misc.Unsafe class would not represent any security issue. It's just about delaying the security check. What do you think? Are there any other security-sensitive j.l.Class methods? Are there any public methods in JDK that take j.l.Class instances and delegate to internal logic assuming that the caller can only pass-in security-pre-checked j.l.Class instances? Regards, Peter David On 2013-09-09, at 10:54 AM, Mandy Chung wrote: Nick, Do you have any information to some of the questions I asked below that can help the API discussion? We need to decide on the permission check and also the hotspot change has to be integrated and the jdk change will need to wait until it shows in a promoted build. Schedule-wise, to make JDK 8, we should finalize the API this week so that you can update the patch soon. Mandy On 9/3/2013 5:02 PM, Mandy Chung wrote: Nick, I skimmed through the changes. Congratulations for your first patch making changes in both hotspot and jdk code. In my mind, the Log4J use case accessing Class instance to obtain additional information for diagnosability is different than the use case of obtaining the caller's class / loader to lookup resources. While the new APIs might be in the same class, I will discuss them individually and keep us focus one at a time. Ralph has pointed out [1] that Log4j also needs the ability to find an appropriate ClassLoader which is used for logging separation (thank you Ralph). That will be the caller-sensitivity (i.e. obtain caller's class/loader) discussion. There are a couple of RFEs: https://bugs.openjdk.java.net/browse/JDK-4942697 https://bugs.openjdk.java.net/browse/JDK-6349551 Performance is critical for Log4j to traverse the stack trace and obtain Class information. I like your patch to speed up the generation of StackTraceElement[] (and StackTraceFrame[] - essentially same code with different type). java.util.logging.LogRecord has workaround the performance overhead and go to a specific frame directly and avoid the cost of generating the entire array. JDK-6349551 requests to lazily instantiate the StackTraceElement object unless that frame is requested. Does Log4J always walk all frames and log all information? Do you just log only some max number of frames rather than the entire stack trace? Class getDeclaringClass() method is the key method you need to enhance the diagnosability. This method opens up a new way to access a Class instance that untrusted code wouldn't be able in the past. A permission check is needed as Alan points out early. Performance as well as logging framework can't access all class loaders are two factors to be considered when defining the permission check. I saw your other comment about permission check (cut-n-paste below). It seems to me that you don't agree a permission check is needed for the getDeclaringClass() method (regardless of which class it belongs to) and if that's the case, no point to continue. I want to make it very clear that I have agreed to take this on and provide a replacement of sun.reflect.Reflection.getCallerClass(int) in JDK 8 to address the use cases. It will take time for the API and security discussion and please be prepared for that (also I am working on other things at the same time). The second comment on your patch is that there are lot of duplicated code in hotspot in order to support two similar but different types (StackTraceFrame and StackTraceElement). Serialization is the reason leading you to have a new StackTraceFrame class. Maybe some refactoring can help but this is the cost of having the VM directly creating the instance. One other option, as suggested in the previous thread, is to
Re: [PATCH] 4851444: Exposing sun.reflect.Reflection#getCallerClass as a public API in Java 8
On 09/09/2013 07:02 PM, David Chase wrote:
Take this lightly informed suggestion with a grain of salt, but why not, for
purposes of performance and security,
change the logging-specific getCallerClass methods so that their "class"
references are instead wrapped in some sort of proxy object that only forwards certain
operations quickly without a security check? For example, equals, hashcode, and toString
are probably not security-sensitive.
i.e.
class SafeClass {
private final Class clazz;
public SafeClass(Class clazz) { this.clazz = class; }
public String toString() { return clazz.toString(); }
public int hashCode() { return clazz.hashCode(); }
public boolean equals(Object o) { return clazz.equals(o); }
public Class maybeWeLetYouLookAtTheRealClass() { ... a bunch of security
checks ... }
}
If necessary, do the same for classloaders.
And them, no security checks needed, as long as the "safe" methods are enough
to get the job done.
Hi,
This is a good idea. It got me thinking that there are a bunch of
methods in j.l.Class that are not security-sensitive. So instead of
proxy-ing those in a wrapper SafeClass, let's just identify "unsafe"
methods 1st. If the security checks that are planned for obtaining
j.l.Class instances from call-stack are transferred to those unsafe
methods instead, then holding a reference to a j.l.Class instance
becomes a security-nonissue.
Just a quick example - presumably the "unsafe" methods of j.l.Class are:
get(Declared)Method(s), get(Declared)Field(s) and
get(Declared)Constructor(s), because they enable you to call/access
public methods/fields/constructors of a class represented by j.l.Class
object. If this class is from a restricted package (say sun.misc) then
you could get access to restricted methods/fields/instances. Now if the
security checks for obtaining reflective object would include checking
the "class visibility/restrictability", then j.l.Class object of say
sun.misc.Unsafe class would not represent any security issue. It's just
about delaying the security check. What do you think? Are there any
other security-sensitive j.l.Class methods? Are there any public methods
in JDK that take j.l.Class instances and delegate to internal logic
assuming that the caller can only pass-in security-pre-checked j.l.Class
instances?
Regards, Peter
David
On 2013-09-09, at 10:54 AM, Mandy Chung wrote:
Nick,
Do you have any information to some of the questions I asked below that can
help the API discussion?
We need to decide on the permission check and also the hotspot change has to be
integrated and the jdk change will need to wait until it shows in a promoted
build. Schedule-wise, to make JDK 8, we should finalize the API this week so
that you can update the patch soon.
Mandy
On 9/3/2013 5:02 PM, Mandy Chung wrote:
Nick,
I skimmed through the changes. Congratulations for your first patch making
changes in both hotspot and jdk code.
In my mind, the Log4J use case accessing Class instance to obtain additional
information for diagnosability is different than the use case of obtaining the
caller's class / loader to lookup resources. While the new APIs might be in
the same class, I will discuss them individually and keep us focus one at a
time.
Ralph has pointed out [1] that Log4j also needs the ability to find an
appropriate ClassLoader which is used for logging separation (thank you Ralph).
That will be the caller-sensitivity (i.e. obtain caller's class/loader)
discussion.
There are a couple of RFEs:
https://bugs.openjdk.java.net/browse/JDK-4942697
https://bugs.openjdk.java.net/browse/JDK-6349551
Performance is critical for Log4j to traverse the stack trace and obtain Class
information. I like your patch to speed up the generation of
StackTraceElement[] (and StackTraceFrame[] - essentially same code with
different type). java.util.logging.LogRecord has workaround the performance
overhead and go to a specific frame directly and avoid the cost of generating
the entire array. JDK-6349551 requests to lazily instantiate the
StackTraceElement object unless that frame is requested. Does Log4J always
walk all frames and log all information? Do you just log only some max number
of frames rather than the entire stack trace?
Class getDeclaringClass() method is the key method you need to enhance the
diagnosability. This method opens up a new way to access a Class instance that
untrusted code wouldn't be able in the past. A permission check is needed as Alan
points out early. Performance as well as logging framework can't access all class
loaders are two factors to be considered when defining the permission check.
I saw your other comment about permission check (cut-n-paste below). It seems
to me that you don't agree a permission check is needed for the
getDeclaringClass() method (regardless of which class it belongs to) and if
that's the case, no point to continue.
I want to make it very clear that I
Re: RFR: 8009411 : getMethods should not inherit static methods from interfaces
On 9 sep 2013, at 19:00, Joel Borggrén-Franck wrote:
> The issue is that since we added static methods to interfaces those have
> erroneously been reflected in getMethods of implementing classes. This fix
> filters out static interface methods from superinterfaces when adding
> methods. I have also added a note to the javadoc for both getMembers and
> getDeclaredMembers pointing this out (though it is implied from JLS).
Correcting myself, I added a note to getMethods() and getMethod(String name …)
getDeclaredMethod{s} shouldn't need a note.
cheers
/Joel
hg: jdk8/tl/jdk: 8024432: Fix doclint issues in java.security
Changeset: be0bcd6a904e Author:juh Date: 2013-09-09 10:52 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/be0bcd6a904e 8024432: Fix doclint issues in java.security Reviewed-by: darcy, mullan ! src/share/classes/java/security/AccessController.java ! src/share/classes/java/security/AlgorithmParameters.java ! src/share/classes/java/security/AlgorithmParametersSpi.java ! src/share/classes/java/security/KeyFactory.java ! src/share/classes/java/security/KeyFactorySpi.java ! src/share/classes/java/security/KeyStore.java ! src/share/classes/java/security/Principal.java ! src/share/classes/java/security/cert/CertPathBuilderSpi.java ! src/share/classes/java/security/cert/CertPathValidatorSpi.java ! src/share/classes/java/security/cert/PKIXRevocationChecker.java ! src/share/classes/java/security/interfaces/RSAMultiPrimePrivateCrtKey.java ! src/share/classes/java/security/interfaces/RSAPrivateCrtKey.java ! src/share/classes/java/security/interfaces/RSAPrivateKey.java ! src/share/classes/java/security/interfaces/RSAPublicKey.java
Re: Please review two corrections for java.time
Hi Peter,
Right, max doesn't solve the issue but I'm not keen on a test that retries
until it gets a better answer.
Adding nanosPerDay if the difference comes out negative would adjust
for the crossing of midnight and not require looping on a more complex
test condition.
The longish delay in the now() method is due to first-time initialization
that reads the timezone data file. Introducing the loop it would change
the test condition so that it is not testing the 'cold' startup.
However, the purpose of the test in not to measure the initialization
overhead
so adding an extra sampling of now(Clock) before the test will remove
the first time
initialization.
Updated webrev at:
http://cr.openjdk.java.net/~rriggs/webrev-localtime-now-8023639/
Thanks, Roger
On 9/9/2013 11:14 AM, Peter Levart wrote:
On 09/09/2013 03:12 PM, roger riggs wrote:
Hi Peter,
The possible wrap-around caused by crossing midnight is handled by
Math.max
so a retry is not needed.
Math.abs(test.toNanoOfDay() - expected.toNanoOfDay())
Hi Roger,
In case there is a wrap-around, the 'diff' is much more than
500,000,000 ns (about 24*60*60*1,000,000,000 ns - delay), which fails
the test.
But what do you think about testing before <= test <= after ? It
should not be timing dependent, like it is now. Does it test the same
thing?
Regards, Peter
Thanks, Roger
On 9/9/2013 2:14 AM, Peter Levart wrote:
On 09/06/2013 07:58 PM, roger riggs wrote:
Please review for two corrections:
- The java/time/tck/java/time/TCKLocalTime test failed on a slow
machine;
the test should be more lenient. The test is not appropriate
for a conformance test
and is moved to java/time/test/java/time/TestLocalTime.
- The javadoc for the JapaneseEra.MEIJI era should indicate the
start date is 1868-01-01
to be consistent with java.util.Calendar. Note that java.time
does not permit dates before Meiji 6
to be created since the calendar is not clearly defined until then.
Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-localtime-now-8023639/
Thanks, Roger
Hi Roger,
Although very in-probable, the test can fail when 'expected' is
sampled before and 'test' is sampled after midnight. I'm guessing
the test is trying to prove that LocalTime.now() is equivalent to
LocalTime.now(Clock.systemDefaultZone()), right?
In that case, what about the following:
public void now() {
LocalTime before, test, after;
do {
before = LocalTime.now(Clock.systemDefaultZone());
test = LocalTime.now();
after = LocalTime.now(Clock.systemDefaultZone());
// retry in case the samples were obtained around midnight
} while (before.compareTo(after) > 0);
assertTrue(before.compareTo(test) <= 0 &&
test.compareTo(after) <= 0);
}
Regards, Peter
hg: jdk8/tl/langtools: 8015322: Javac template test framework
Changeset: 67c5110c60fe Author:emc Date: 2013-09-09 17:11 -0400 URL: http://hg.openjdk.java.net/jdk8/tl/langtools/rev/67c5110c60fe 8015322: Javac template test framework Summary: Putback of the javac template test framework from the Lambda repository Reviewed-by: jjg Contributed-by: brian.go...@oracle.com ! README + test/lib/combo/TEST.properties + test/lib/combo/tools/javac/combo/Diagnostics.java + test/lib/combo/tools/javac/combo/JavacTemplateTestBase.java + test/lib/combo/tools/javac/combo/Template.java + test/lib/combo/tools/javac/combo/TemplateTest.java + test/tools/javac/lambda/bridge/template_tests/BridgeMethodTestCase.java + test/tools/javac/lambda/bridge/template_tests/BridgeMethodsTemplateTest.java + test/tools/javac/lambda/bridge/template_tests/TEST.properties
Re: RFR: 8011916: Spec update for java.util.stream + 8024339: j.u.s.Stream.reduce(BinaryOperator) throws unexpected NPE
Looks like good changes all around. I didn't see any "breaking" changes. Some of the javadoc examples ( blocks) are wrapped at 80 columns. I have been allowing them to go wider (110 columns roughly) so that they don't end up being very narrow in the HTML output. Mike On Sep 6 2013, at 23:18 , Henry Jen wrote: > Hi, > > Please kindly review webrev at > http://cr.openjdk.java.net/~henryjen/ccc/8011916/0/webrev/ > > The webrev brings over the latest javadoc overhaul occurred in lambda > repo. The specdiff can be found at > > http://cr.openjdk.java.net/~henryjen/ccc/8011916/0/specdiff/overview-summary.html > > There is also some minor cleanup for code, variable renaming, add > @Override etc. > > Bug 8024339 is resolved with spec updated to clarify NPE could be thrown > if reduce operation got a null as result. > > Cheers, > Henry >
Re: RFR: 8009411 : getMethods should not inherit static methods from interfaces
On 09/09/2013 07:27 PM, Joel Borggrén-Franck wrote:
On 9 sep 2013, at 19:00, Joel Borggrén-Franck wrote:
The issue is that since we added static methods to interfaces those have
erroneously been reflected in getMethods of implementing classes. This fix
filters out static interface methods from superinterfaces when adding methods.
I have also added a note to the javadoc for both getMembers and
getDeclaredMembers pointing this out (though it is implied from JLS).
Correcting myself, I added a note to getMethods() and getMethod(String name …)
getDeclaredMethod{s} shouldn't need a note.
cheers
/Joel
also Joel you can use for-each and avoid to load ma[i] twice
(even if the JIT will certainly avoid the double load in this specific
case).
void addAllNonStatic(Method[] methodArray) {
for (Method method:methodArray) {
if (!Modifier.isStatic(method.getModifiers())) {
add(method);
}
}
}
cheers,
Rémi
hg: jdk8/tl/langtools: 8022322: Reject default and static methods in annotation
Changeset: f4efd6ef6e80 Author:emc Date: 2013-09-09 16:26 -0400 URL: http://hg.openjdk.java.net/jdk8/tl/langtools/rev/f4efd6ef6e80 8022322: Reject default and static methods in annotation Summary: Causes javac to reject static and default method declarations inside an annotation Reviewed-by: jjg ! src/share/classes/com/sun/tools/javac/code/Flags.java ! src/share/classes/com/sun/tools/javac/comp/Check.java + test/tools/javac/annotations/neg/NoDefault.java + test/tools/javac/annotations/neg/NoDefault.out + test/tools/javac/annotations/neg/NoDefaultAbstract.java + test/tools/javac/annotations/neg/NoDefaultAbstract.out + test/tools/javac/annotations/neg/NoStatic.java + test/tools/javac/annotations/neg/NoStatic.out + test/tools/javac/annotations/neg/NoStaticAbstract.java + test/tools/javac/annotations/neg/NoStaticAbstract.out
JDK-6962494: Update documentation on Executable.getParameterAnnotations()
Hello, Please review this patch which updates the javadoc comments for java.lang.reflect.Executable.getParameterAnnotations(). The patch corrects the javadocs to describe the actual behavior of this method. It also refers users to the new java.lang.reflect.Parameter API. See comments on the bug report for more details: https://bugs.openjdk.java.net/browse/JDK-6962494 The webrev is here: http://cr.openjdk.java.net/~emc/6962494/ This is also under review in crucible. The review link is here: http://sthinfra10.se.oracle.com:8060/cru/CR-JDK8TL-171 Thanks, Eric
Re: [PATCH] 4851444: Exposing sun.reflect.Reflection#getCallerClass as a public API in Java 8
On 9/9/13 10:02 AM, David Chase wrote:
Take this lightly informed suggestion with a grain of salt, but why not, for
purposes of performance and security,
change the logging-specific getCallerClass methods so that their "class"
references are instead wrapped in some sort of proxy object that only forwards certain
operations quickly without a security check? For example, equals, hashcode, and toString
are probably not security-sensitive.
Most of the information obtained from a class the use cases are
interested in are security-sensitive information (e.g. protection
domain, code source, class loader).
Mandy
i.e.
class SafeClass {
private final Class clazz;
public SafeClass(Class clazz) { this.clazz = class; }
public String toString() { return clazz.toString(); }
public int hashCode() { return clazz.hashCode(); }
public boolean equals(Object o) { return clazz.equals(o); }
public Class maybeWeLetYouLookAtTheRealClass() { ... a bunch of security
checks ... }
}
If necessary, do the same for classloaders.
And them, no security checks needed, as long as the "safe" methods are enough
to get the job done.
David
On 2013-09-09, at 10:54 AM, Mandy Chung wrote:
Nick,
Do you have any information to some of the questions I asked below that can
help the API discussion?
We need to decide on the permission check and also the hotspot change has to be
integrated and the jdk change will need to wait until it shows in a promoted
build. Schedule-wise, to make JDK 8, we should finalize the API this week so
that you can update the patch soon.
Mandy
On 9/3/2013 5:02 PM, Mandy Chung wrote:
Nick,
I skimmed through the changes. Congratulations for your first patch making
changes in both hotspot and jdk code.
In my mind, the Log4J use case accessing Class instance to obtain additional
information for diagnosability is different than the use case of obtaining the
caller's class / loader to lookup resources. While the new APIs might be in
the same class, I will discuss them individually and keep us focus one at a
time.
Ralph has pointed out [1] that Log4j also needs the ability to find an
appropriate ClassLoader which is used for logging separation (thank you Ralph).
That will be the caller-sensitivity (i.e. obtain caller's class/loader)
discussion.
There are a couple of RFEs:
https://bugs.openjdk.java.net/browse/JDK-4942697
https://bugs.openjdk.java.net/browse/JDK-6349551
Performance is critical for Log4j to traverse the stack trace and obtain Class
information. I like your patch to speed up the generation of
StackTraceElement[] (and StackTraceFrame[] - essentially same code with
different type). java.util.logging.LogRecord has workaround the performance
overhead and go to a specific frame directly and avoid the cost of generating
the entire array. JDK-6349551 requests to lazily instantiate the
StackTraceElement object unless that frame is requested. Does Log4J always
walk all frames and log all information? Do you just log only some max number
of frames rather than the entire stack trace?
Class getDeclaringClass() method is the key method you need to enhance the
diagnosability. This method opens up a new way to access a Class instance that
untrusted code wouldn't be able in the past. A permission check is needed as Alan
points out early. Performance as well as logging framework can't access all class
loaders are two factors to be considered when defining the permission check.
I saw your other comment about permission check (cut-n-paste below). It seems
to me that you don't agree a permission check is needed for the
getDeclaringClass() method (regardless of which class it belongs to) and if
that's the case, no point to continue.
I want to make it very clear that I have agreed to take this on and provide a
replacement of sun.reflect.Reflection.getCallerClass(int) in JDK 8 to address
the use cases. It will take time for the API and security discussion and
please be prepared for that (also I am working on other things at the same
time).
The second comment on your patch is that there are lot of duplicated code in
hotspot in order to support two similar but different types (StackTraceFrame
and StackTraceElement). Serialization is the reason leading you to have a new
StackTraceFrame class. Maybe some refactoring can help but this is the cost of
having the VM directly creating the instance. One other option, as suggested
in the previous thread, is to keep the declaring class in the StackTraceElement
as a transient field. If we add the getDeclaringClass method in the
StackTraceElement class, it would need to specify to be optional that it only
returns the Class instance when it's available.
There are currently three different ways to get a stack trace:
1. Throwable.getStackTrace()
2. Thread.getStackTrace() and Thread.getAllStackTraces()
3. java.lang.management.ThreadMXBean.getThreadInfo(long id, int
maxDepth).getStackTrace() and multiple thread
Re: [PATCH] 4851444: Exposing sun.reflect.Reflection#getCallerClass as a public API in Java 8
On 9/9/13 10:01 AM, David M. Lloyd wrote: I'm not sure if we can be very certain about the depth in a runtime environment (non-debugging) unless it requires all VM implementation to support a reliable way to return a frame at a given depth. The stack trace is not guaranteed to contain all stack frames. E.g. in the spec of Throwable.getStackTrace(): Some virtual machines may, under some circumstances, omit one or more stack frames from the stack trace. In the extreme case, a virtual machine that has no stack trace information concerning this throwable is permitted to return a zero-length array from this method. This is interesting. Presumably this would tie into tail-call optimizations and that sort of thing? How does AccessControlContext deal with this possibility? Will need the VM team to answer the details. ACC only needs protection domains that allow the VM to do some ACC optimization, for example, the comment in JVM_GetStackAccessControlContext // count the protection domains on the execution stack. We collapse // duplicate consecutive protection domains into a single one, as // well as stopping when we hit a privileged frame. Also classes on bootclasspath have null protection domain in the VM itself that it can bypass permission check on those frames. Mandy
hg: jdk8/tl/jdk: 8023447: change specification to allow RMI activation to be optional
Changeset: 6731ea9123f2 Author:smarks Date: 2013-09-09 14:11 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/6731ea9123f2 8023447: change specification to allow RMI activation to be optional Reviewed-by: darcy, alanb, olagneau ! src/share/classes/java/rmi/activation/Activatable.java ! src/share/classes/java/rmi/activation/ActivationDesc.java ! src/share/classes/java/rmi/activation/ActivationGroup.java ! src/share/classes/java/rmi/activation/ActivationGroupID.java ! src/share/classes/java/rmi/activation/ActivationID.java ! src/share/classes/java/rmi/activation/package.html
Re: RFR: 8023340 : (xxs) Clarify that unmodifiable List.replaceAll() may not throw UOE if there are no items to be replaced.
Responding to both of your messages at once.
On Sep 5 2013, at 21:49 , David Holmes wrote:
> Hi Mike,
>
> On 6/09/2013 2:58 AM, Mike Duigou wrote:
>> Hello all;
>>
>> A very small spec clarification for review. The spec clarification ensures
>> that the behaviour of the default method can satisfy the needs of
>> unmodifiable implementations.
>>
>> http://cr.openjdk.java.net/~mduigou/JDK-8023340/0/webrev/
>
> I don't agree with the change. An Unmodifable list _should_ throw UOE.
>
> That aside I don't know how to interpret you change:
>
> "operation is not supported by this list for some element"
>
> What does "for some element" mean ???
>
> David
>
>> Thanks,
>>
>> Mike
>>
On Sep 6 2013, at 08:35 , Martin Buchholz wrote:
> ListIterator.set is specified to throw these exceptions:
>
> * @throws UnsupportedOperationException if the {@code set} operation
> * is not supported by this list iterator
> * @throws ClassCastException if the class of the specified element
> * prevents it from being added to this list
> * @throws IllegalArgumentException if some aspect of the specified
> * element prevents it from being added to this list
> * @throws IllegalStateException if neither {@code next} nor
> * {@code previous} have been called, or {@code remove} or
> * {@code add} have been called after the last call to
> * {@code next} or {@code previous}
>
> List.replaceAll invokes set (conceptually), so it needs to specify the same
> exceptions, no? Many years ago I made an effort to make these sorts of
> exception specs more consistent and accurate - a good outlet for my inner
> pedant. Perhaps that needs to be done once more? Hmmm. I notice the
> above list is missing NullPointerException; that looks like a bug... (my bad,
> I think)
>
> Like David, I don't see the point of this change.
The goal is to allow Collections.emptyList(), Collections.singletonList() and
possibly other implementations to use the default List.replaceAll
implementation. I want to avoid having to override the default implementation
in order to throw UOE. In the case of emptyList() I justify this this by saying
"the list is empty, so replaceAll matches nothing, no need to throw UOE". For
singletonList() my goal is to allow the ListIterator.set() to throw the UOE
exception rather than having to write a UOE throwing replaceAll() method for
Collections.singletonList().
I significant motivation is that I want to ensure that the specification for
the defaulted method is written such that the default can satisfy situations
such as empty or unmodifiable collection--I could "fix"
Collections.singletonList() but I can't fix every possible unmodifiable list
and the default implementation can't a priori know that calling the
ListIterator.set() will or won't throw UOE.
> UOE is not supposed to be at the element level. If a List or ListIterator
> doesn't like a particular element, it should be throwing one of the above
> exceptions (plus NPE).
Yeah, probably a mistake on my part. I will try again at a definition for
replaceAll() that avoids having to write override and is still coherent.
Thanks,
Mike
>
>
>
> On Thu, Sep 5, 2013 at 9:58 AM, Mike Duigou wrote:
> Hello all;
>
> A very small spec clarification for review. The spec clarification ensures
> that the behaviour of the default method can satisfy the needs of
> unmodifiable implementations.
>
> http://cr.openjdk.java.net/~mduigou/JDK-8023340/0/webrev/
>
> Thanks,
>
> Mike
>
Re: [PATCH] 4851444: Exposing sun.reflect.Reflection#getCallerClass as a public API in Java 8
On 09/09/2013 04:56 PM, Mandy Chung wrote: On 9/9/13 10:01 AM, David M. Lloyd wrote: I'm not sure if we can be very certain about the depth in a runtime environment (non-debugging) unless it requires all VM implementation to support a reliable way to return a frame at a given depth. The stack trace is not guaranteed to contain all stack frames. E.g. in the spec of Throwable.getStackTrace(): Some virtual machines may, under some circumstances, omit one or more stack frames from the stack trace. In the extreme case, a virtual machine that has no stack trace information concerning this throwable is permitted to return a zero-length array from this method. This is interesting. Presumably this would tie into tail-call optimizations and that sort of thing? How does AccessControlContext deal with this possibility? Will need the VM team to answer the details. ACC only needs protection domains that allow the VM to do some ACC optimization, for example, the comment in JVM_GetStackAccessControlContext // count the protection domains on the execution stack. We collapse // duplicate consecutive protection domains into a single one, as // well as stopping when we hit a privileged frame. Also classes on bootclasspath have null protection domain in the VM itself that it can bypass permission check on those frames. Leaving the bootclasspath stuff aside, it would be very useful for (at least) security managers to be able to acquire *just* their immediate caller's ProtectionDomain without having to worry about spoofing or anything like that. This would definitely solve my security manager use case at the very least - it would allow writing privileged versions of methods which would not require a slow doPrivileged() call; instead I could just check against the immediate caller's permission set. -- - DML
hg: jdk8/tl/langtools: 8019521: Enhanced rethrow disabled in lambdas
Changeset: 77d395862700 Author:jlahoda Date: 2013-09-09 23:13 +0200 URL: http://hg.openjdk.java.net/jdk8/tl/langtools/rev/77d395862700 8019521: Enhanced rethrow disabled in lambdas Summary: Fixing effectively final detection inside lambdas, small cleanup related to thrown types detection in lambdas Reviewed-by: mcimadamore, jjg ! src/share/classes/com/sun/tools/javac/comp/Attr.java ! src/share/classes/com/sun/tools/javac/comp/Flow.java ! src/share/classes/com/sun/tools/javac/tree/JCTree.java + test/tools/javac/lambda/EffectivelyFinalThrows.java
Re: Please review two corrections for java.time
Hi Roger, On 9/09/2013 10:48 PM, roger riggs wrote: Hi David, I found even on my VirturalBox machine it frequently came close to the .1ms target and failed in one case. Raising the time was to reduce/prevent intermittent failures. Are other timing tests also sensitive to the Xcomp, how should tests be written to be insensitive to that JVM option? Xcomp can be very detrimental to timing. What _should_ happen with timeout/delay factors in tests (if they are unavoidable) is that a base delay should be chosen and it should be multiplied by a scaling factor that can be passed in via the test harness based on the execution environment (slow machine, use of Xcomp etc). This is rarely done however. :( I'm not sure what the JCK mechanism would be for that. Are you otherwise ok with the changes? I can only comment on the timeout change. David Thanks, Roger On 9/8/2013 10:43 PM, David Holmes wrote: Hi Roger, On 7/09/2013 3:58 AM, roger riggs wrote: Please review for two corrections: - The java/time/tck/java/time/TCKLocalTime test failed on a slow machine; the test should be more lenient. The test is not appropriate for a conformance test and is moved to java/time/test/java/time/TestLocalTime. As per the bug report the issue is not slow machines per-se but the use of Xcomp when running the test. I don't think the jck should ever be run with Xcomp. It will be interesting to see if the change from 100ms to 500ms cures the problem on all machines. Thanks, David - The javadoc for the JapaneseEra.MEIJI era should indicate the start date is 1868-01-01 to be consistent with java.util.Calendar. Note that java.time does not permit dates before Meiji 6 to be created since the calendar is not clearly defined until then. Webrev: http://cr.openjdk.java.net/~rriggs/webrev-localtime-now-8023639/ Thanks, Roger
hg: jdk8/tl/langtools: 8006972: jtreg test fails: test/tools/javac/processing/model/element/TestMissingElement/TestMissingElement.java
Changeset: 7439356a7dc5 Author:jjg Date: 2013-09-09 17:36 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/langtools/rev/7439356a7dc5 8006972: jtreg test fails: test/tools/javac/processing/model/element/TestMissingElement/TestMissingElement.java Reviewed-by: darcy ! test/tools/javac/processing/model/element/TestMissingElement/TestMissingElement.java ! test/tools/javac/processing/model/element/TestMissingElement/TestMissingElement.ref
hg: jdk8/tl/jdk: 8024444: Change to use othervm mode of tests in SSLEngineImpl
Changeset: f80b8af9c218 Author:xuelei Date: 2013-09-09 19:07 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/f80b8af9c218 802: Change to use othervm mode of tests in SSLEngineImpl Reviewed-by: mullan ! test/sun/security/ssl/com/sun/net/ssl/internal/ssl/SSLEngineImpl/CloseEngineException.java ! test/sun/security/ssl/com/sun/net/ssl/internal/ssl/SSLEngineImpl/CloseInboundException.java ! test/sun/security/ssl/com/sun/net/ssl/internal/ssl/SSLEngineImpl/CloseStart.java ! test/sun/security/ssl/com/sun/net/ssl/internal/ssl/SSLEngineImpl/DelegatedTaskWrongException.java ! test/sun/security/ssl/com/sun/net/ssl/internal/ssl/SSLEngineImpl/EmptyExtensionData.java ! test/sun/security/ssl/com/sun/net/ssl/internal/ssl/SSLEngineImpl/EngineEnforceUseClientMode.java ! test/sun/security/ssl/com/sun/net/ssl/internal/ssl/SSLEngineImpl/RehandshakeFinished.java
Re: Please review two corrections for java.time
Typo:
+ assertTrue(diff < 1); // less than 0.5 secs
David
On 10/09/2013 5:42 AM, roger riggs wrote:
Hi Peter,
Right, max doesn't solve the issue but I'm not keen on a test that retries
until it gets a better answer.
Adding nanosPerDay if the difference comes out negative would adjust
for the crossing of midnight and not require looping on a more complex
test condition.
The longish delay in the now() method is due to first-time initialization
that reads the timezone data file. Introducing the loop it would change
the test condition so that it is not testing the 'cold' startup.
However, the purpose of the test in not to measure the initialization
overhead
so adding an extra sampling of now(Clock) before the test will remove
the first time
initialization.
Updated webrev at:
http://cr.openjdk.java.net/~rriggs/webrev-localtime-now-8023639/
Thanks, Roger
On 9/9/2013 11:14 AM, Peter Levart wrote:
On 09/09/2013 03:12 PM, roger riggs wrote:
Hi Peter,
The possible wrap-around caused by crossing midnight is handled by
Math.max
so a retry is not needed.
Math.abs(test.toNanoOfDay() - expected.toNanoOfDay())
Hi Roger,
In case there is a wrap-around, the 'diff' is much more than
500,000,000 ns (about 24*60*60*1,000,000,000 ns - delay), which fails
the test.
But what do you think about testing before <= test <= after ? It
should not be timing dependent, like it is now. Does it test the same
thing?
Regards, Peter
Thanks, Roger
On 9/9/2013 2:14 AM, Peter Levart wrote:
On 09/06/2013 07:58 PM, roger riggs wrote:
Please review for two corrections:
- The java/time/tck/java/time/TCKLocalTime test failed on a slow
machine;
the test should be more lenient. The test is not appropriate
for a conformance test
and is moved to java/time/test/java/time/TestLocalTime.
- The javadoc for the JapaneseEra.MEIJI era should indicate the
start date is 1868-01-01
to be consistent with java.util.Calendar. Note that java.time
does not permit dates before Meiji 6
to be created since the calendar is not clearly defined until then.
Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-localtime-now-8023639/
Thanks, Roger
Hi Roger,
Although very in-probable, the test can fail when 'expected' is
sampled before and 'test' is sampled after midnight. I'm guessing
the test is trying to prove that LocalTime.now() is equivalent to
LocalTime.now(Clock.systemDefaultZone()), right?
In that case, what about the following:
public void now() {
LocalTime before, test, after;
do {
before = LocalTime.now(Clock.systemDefaultZone());
test = LocalTime.now();
after = LocalTime.now(Clock.systemDefaultZone());
// retry in case the samples were obtained around midnight
} while (before.compareTo(after) > 0);
assertTrue(before.compareTo(test) <= 0 &&
test.compareTo(after) <= 0);
}
Regards, Peter
Re: Please review two corrections for java.time
On 09/09/2013 09:42 PM, roger riggs wrote:
Hi Peter,
Right, max doesn't solve the issue but I'm not keen on a test that
retries
until it gets a better answer.
Hi Roger,
If java.time logic is correct, it should only ever retry once when
roll-over or DST jump-back happens, so the test could be made to fail if
it tries to retry the 2nd time, indicating unexpected behaviour. The
"jumps" in LocalTime should be very far-apart so the test should only
encounter one of them, if any.
Adding nanosPerDay if the difference comes out negative would adjust
for the crossing of midnight and not require looping on a more complex
test condition.
That's ok for midnight roll-over, but what about DST jumps? They only
happen two times a year, so you expect the test will never encounter them?
Regards, Peter
The longish delay in the now() method is due to first-time initialization
that reads the timezone data file. Introducing the loop it would change
the test condition so that it is not testing the 'cold' startup.
However, the purpose of the test in not to measure the initialization
overhead
so adding an extra sampling of now(Clock) before the test will remove
the first time
initialization.
Updated webrev at:
http://cr.openjdk.java.net/~rriggs/webrev-localtime-now-8023639/
Thanks, Roger
On 9/9/2013 11:14 AM, Peter Levart wrote:
On 09/09/2013 03:12 PM, roger riggs wrote:
Hi Peter,
The possible wrap-around caused by crossing midnight is handled by
Math.max
so a retry is not needed.
Math.abs(test.toNanoOfDay() - expected.toNanoOfDay())
Hi Roger,
In case there is a wrap-around, the 'diff' is much more than
500,000,000 ns (about 24*60*60*1,000,000,000 ns - delay), which fails
the test.
But what do you think about testing before <= test <= after ? It
should not be timing dependent, like it is now. Does it test the same
thing?
Regards, Peter
Thanks, Roger
On 9/9/2013 2:14 AM, Peter Levart wrote:
On 09/06/2013 07:58 PM, roger riggs wrote:
Please review for two corrections:
- The java/time/tck/java/time/TCKLocalTime test failed on a slow
machine;
the test should be more lenient. The test is not appropriate
for a conformance test
and is moved to java/time/test/java/time/TestLocalTime.
- The javadoc for the JapaneseEra.MEIJI era should indicate the
start date is 1868-01-01
to be consistent with java.util.Calendar. Note that java.time
does not permit dates before Meiji 6
to be created since the calendar is not clearly defined until then.
Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-localtime-now-8023639/
Thanks, Roger
Hi Roger,
Although very in-probable, the test can fail when 'expected' is
sampled before and 'test' is sampled after midnight. I'm guessing
the test is trying to prove that LocalTime.now() is equivalent to
LocalTime.now(Clock.systemDefaultZone()), right?
In that case, what about the following:
public void now() {
LocalTime before, test, after;
do {
before = LocalTime.now(Clock.systemDefaultZone());
test = LocalTime.now();
after = LocalTime.now(Clock.systemDefaultZone());
// retry in case the samples were obtained around midnight
} while (before.compareTo(after) > 0);
assertTrue(before.compareTo(test) <= 0 &&
test.compareTo(after) <= 0);
}
Regards, Peter
