Re: JDK-8210280 - Unnecessary reallocation when invoking HashMap.putAll()

2018-12-12 Thread Martin Buchholz
Software is hard. I found myself removing the remaining style changes to be able to review the changes. (We're going to have to disagree about the value of curlies). Here's my reduction: --- src/main/java/util/HashMap.java 11 Nov 2018 16:27:28 - 1.9 +++ src/main/java/util/HashMap.java 12 Dec

Re: RFR(s): JDK-8214687 Optimize Collections.nCopies().hashCode()

2018-12-11 Thread Martin Buchholz
к. 2018 г. в 11:02, Martin Buchholz : > >> In performance critical code, we don't trust hotspot to not reload final >>> fields. Other forEach methods do this, e.g. >> >> >> final Object[] es = queue; >> for (int i = 0, n = size; i < n; i++) >> action.accept((E) es[i]); >> >> >

Re: RFR(s): JDK-8214687 Optimize Collections.nCopies().hashCode()

2018-12-11 Thread Martin Buchholz
> > In performance critical code, we don't trust hotspot to not reload final > fields. Other forEach methods do this, e.g. final Object[] es = queue; for (int i = 0, n = size; i < n; i++) action.accept((E) es[i]);

Re: JDK-8210280 - Unnecessary reallocation when invoking HashMap.putAll()

2018-12-11 Thread Martin Buchholz
Hi Michal, pleased to meet you. I'll be your sponsor. The test will need a legal header, presumably similar to others authored by redhatters. It is now possible to check in jmh microbenchmarks (but I've never done so myself). The current coding style is non-standard, but deliberate; avoid

Re: RFR: 8215159: Improve initial setup of system Properties

2018-12-11 Thread Martin Buchholz
HashMap sizing was a terrible design mistake, but too much software depends on it, e.g. https://google.github.io/guava/releases/23.0/api/docs/com/google/common/collect/Maps.html#newHashMapWithExpectedSize-int- On Tue, Dec 11, 2018 at 1:26 AM Claes Redestad wrote: > Hi Peter, > > On 2018-12-11

Re: RFR: 8215017: Improve String::equals warmup characteristics

2018-12-10 Thread Martin Buchholz
Claes, make sure you've mind-melded with Aleksey on this, e.g. via https://www.youtube.com/watch?v=wIyeOaitmWM On Fri, Dec 7, 2018 at 4:07 PM Claes Redestad wrote: > Hi, > > following up from discussions during review of JDK-8214971[1], I > examined the startup and peak performance of a few

RFR: jsr166 integration 2018-12

2018-12-08 Thread Martin Buchholz
Last jdk12 jsr166 integration. https://cr.openjdk.java.net/~martin/webrevs/jdk/jsr166-integration/overview.html 8214559: Use {@systemProperty} for definitions of system properties https://cr.openjdk.java.net/~martin/webrevs/jdk/jsr166-integration/systemProperty/index.html

RFR: 8215048: Some classloader typos

2018-12-08 Thread Martin Buchholz
This is all I got for jdk12: https://cr.openjdk.java.net/~martin/webrevs/jdk/classloader-typos/ https://bugs.openjdk.java.net/browse/JDK-8215048

Re: RFR 8214794 : java.specification.version should be only the major version number

2018-12-04 Thread Martin Buchholz
> > LGTM

Re: RFR 8214794 : java.specification.version should be only the major version number

2018-12-04 Thread Martin Buchholz
> > I would make a stronger assertion, that Runtime.version().feature() > converted to a String is equal to specVersion, catching bogus specVersions > like "+011"

Re: RFR: XXXS: JDK-8214745: Bad link in coll-reference.html

2018-12-03 Thread Martin Buchholz
LGTM On Mon, Dec 3, 2018 at 4:27 PM Jonathan Gibbons wrote: > Please review a tiny patch, given below, to fix a broken link in > coll-reference.html. The problem is bad/misplaced punctuation characters > within the link. This is believed to be the last broken link in the > java.base module, and

Re: RFR: [XXS] 8214744: Unnecessary tags in java.util.zip.Deflater

2018-12-03 Thread Martin Buchholz
LGTM On Mon, Dec 3, 2018 at 4:07 PM Jonathan Gibbons wrote: > Please review this small patch, given below, to remove unnecessary > reported by `tidy` as the last remaining HTML issues in the > documentation for java.base > >

Re: Optimized version of CopiesList.hashCode()

2018-11-30 Thread Martin Buchholz
On Thu, Nov 29, 2018 at 8:02 PM, Tagir Valeev wrote: > > I can file an issue and create a webrev, but I still need a sponsor > and review for such change. Martin, can you help with this? > As usual, I'm only half paying attention, but I can be your shepherd. You're optimizing hashCode to be

Re: RFR: 8214077: test java/io/File/SetLastModified.java fails on ARM32

2018-11-27 Thread Martin Buchholz
On Tue, Nov 27, 2018 at 7:25 PM, Nick Gasson wrote: > > I missed one thing then looking at this. In TimeZone_md.c it should be > > #ifdef MACOSX rather than #ifndef. I can sponsor the change for you but > > I need to change this one line before pushing. > > Hi Alan, > > Yes feel free to modify

RFR: jsr166 integration 2018-11

2018-11-26 Thread Martin Buchholz
https://cr.openjdk.java.net/~martin/webrevs/jdk/jsr166-integration/overview.html Another dull jsr166 integration (last one for jdk12?) 8212899: java/util/concurrent/tck/JSR166TestCase.java - testMissedSignal_8187947(SubmissionPublisherTest): timed out waiting for CountDownLatch for 40 sec

Re: RFR: 8212794 IBM-964 and IBM-29626C are required for AIX default charset

2018-11-26 Thread Martin Buchholz
Sorry, I'm sympathetic but I can't help push this through. These days, all computers should have a default charset of UTF-8 or perhaps one of the other "Standard" charsets. Other charsets should be regarded as legacy. Surely AIX has UTF-8 variants of all the locales? On Sun, Nov 25, 2018 at

Re: Optimized version of CopiesList.hashCode()

2018-11-26 Thread Martin Buchholz
I agree! (but don't have time ...) On Sun, Nov 25, 2018 at 9:01 PM, Zheka Kozlov wrote: > Currently, CopiesList.hashCode() is inherited from AbstractList which: > >- calls hashCode() for each element, >- creates a new Iterator every time. > > However, for Collections.nCopies(): > >

Re: Re: RFR: 8212794 IBM-964 and IBM-29626C are required for AIX default charset

2018-11-14 Thread Martin Buchholz
Ichiroh, IBM/AIX people should reach consensus on how to handle IBM charsets. I'm going to add some IBM people who might know or care. (I haven't used AIX in 20 years) On Wed, Nov 14, 2018 at 10:23 AM, Ichiroh Takiguchi < taki...@linux.vnet.ibm.com> wrote: > Hello. > >

Re: RFR: 8213085: (tz) Upgrade time-zone data to tzdata2018g

2018-10-29 Thread Martin Buchholz
LGTM. On Mon, Oct 29, 2018 at 8:19 AM, Ramanand Patil wrote: > Hi all, > Please review the latest TZDATA integration (tzdata2018g) into JDK12. > Bug: https://bugs.openjdk.java.net/browse/JDK-8213085 > Webrev: http://cr.openjdk.java.net/~rpatil/8213085/webrev.00/ > > All the TimeZone related

Re: Time-zone database issues

2018-10-28 Thread Martin Buchholz
Aligning the openjdk API with the tzdb format is a good thing to do long term. However, updating old jdk versions is sufficiently difficult (and some people will want to keep jdk6 and jdk9 tzdata up to date for years) that I'm already resigned to using rearguard format "forever". It's a simple

Re: Request Review: JDK-8213043: Add internal Unsafe xxxObject methods as jsr166 is broken

2018-10-26 Thread Martin Buchholz
Thanks for doing this - I tested the patch and it'll work for us. On Fri, Oct 26, 2018 at 8:46 AM, Mandy Chung wrote: > jsr166 depends on the internal Unsafe xxxObject methods and is currently > broken. Since the rename is motivated by Valhalla, we agree to keep the > internal Unsafe xxxObject

Re: RFR: 8213016: (tz) Upgrade time-zone data to tzdata2018f

2018-10-26 Thread Martin Buchholz
Looks good to me. On Fri, Oct 26, 2018 at 11:30 AM, Ramanand Patil wrote: > Hi all, > Please review the latest TZDATA integration (tzdata2018f) into JDK12. > Bug: https://bugs.openjdk.java.net/browse/JDK-8213016 > Webrev: http://cr.openjdk.java.net/~rpatil/8213016/webrev.00/ > > All the

Re: Exceptions in Iterator.forEachRemaining()

2018-10-26 Thread Martin Buchholz
Mostly my fault, and even more inconsistent than you say. I filed https://bugs.openjdk.java.net/browse/JDK-8213038 (but don't use an iterator after a call to forEachRemaining!) On Thu, Oct 25, 2018 at 11:55 PM, Zheka Kozlov wrote: > Hi everyone. > > I noticed that different Iterator

Re: Approval Request to update TreeSet#add method documentation

2018-10-21 Thread Martin Buchholz
va file for more details. > > @Martin : I really appreciate your advice and help, thank you. > > Thank you, > Kishor Gollapalliwar > > On Sat, Oct 13, 2018 at 11:40 PM Martin Buchholz > wrote: > > > core-libs-dev is the right mailing list. > > > > The d

Re: 6850720: Allow POSIX_SPAWN to be used for ProcessImpl on Linux

2018-10-21 Thread Martin Buchholz
As author of the vfork strategy ... I'm supportive of the directions in this thread. David's patch seems like clear progress (although maybe now that we have configure, we can make the spawn strategy conditional on HAVE_SPAWN_H) vfork is even (!) less in favor than it used to be, so migrating

Re: Review Request: 6202130: java.util.jar.Attributes.writeMain() can't handle multi-byte chars

2018-10-21 Thread Martin Buchholz
vised and updated patch attached. > > Regards, > Philipp > > > > On Thu, 2018-10-04 at 15:24 -0700, Martin Buchholz wrote: > > "utf8" is a thing. "utf" is not. > > > > On Thu, Oct 4, 2018 at 12:58 PM, Philipp Kunz > h> wrote: > > > Th

Re: Inconsistencies in TreeSet Interface

2018-10-13 Thread Martin Buchholz
core-libs-dev is the right mailing list. The documentation could be improved, but it's nuanced - consider using a TreeSet with a Comparator that accepts null. On Sat, Oct 13, 2018 at 7:26 AM, Kishor Gollapalliwar < kishor.gollapalli...@gmail.com> wrote: > Hello Everyone, > > Introduction : I’m

Re: Why Stream.concat is a static method - type variable contravariance

2018-10-11 Thread Martin Buchholz
Is this the problem discussed here? http://www.angelikalanger.com/GenericsFAQ/FAQSections/TypeParameters.html#Lower%20Bound%20Type%20Parameters%20of%20Methods (I think I ran into this myself and think the java language should be fixed to allow "Lower Bound Type Parameters of Methods") On Wed, Oct

Re: RFR JDK-8211880,Broken links in java.util.jar

2018-10-09 Thread Martin Buchholz
LGTM On Tue, Oct 9, 2018 at 12:07 PM, Xueming Shen wrote: > Hi, > > Please help review the doc-link-only update for JDK-8211880 > > issue: https://bugs.openjdk.java.net/browse/JDK-8211880 > webrev: http://cr.openjdk.java.net/~sherman/8211880/webrev/ > > Thanks, > Sherman >

Re: Review Request: 6202130: java.util.jar.Attributes.writeMain() can't handle multi-byte chars

2018-10-04 Thread Martin Buchholz
"utf8" is a thing. "utf" is not. On Thu, Oct 4, 2018 at 12:58 PM, Philipp Kunz wrote: > Thanks to Sherman's hint, I revised the test to use the terms unicode > character and utf encoding consistently and not utf character. Affects > only the test, mostly in comments and a few identifiers. > >

Re: [PATCH] 4511638: Double.toString(double) sometimes produces incorrect results

2018-10-02 Thread Martin Buchholz
Raffaello and Ulf should talk. It seems like Ulf's ryu project is trying to solve the same problem. ryu is also still being worked on, but there is already a published paper and ryu is being adopted by various core libraries. https://github.com/ulfjack/ryu

Re: Convert old-style array declarations (was: Re: ByteArrayOutputStream should not have a new writeBytes method in Java)

2018-10-01 Thread Martin Buchholz
Thank you very much for working on these cleanups. I've occasionally done similar in the past. I've made big changesets with only one automated change at a time. So e.g. I would do only "C-style array declaration" in one changeset. This is one example of a change that should leave zero impact

Re: Optimized HashSet.toArray()

2018-10-01 Thread Martin Buchholz
Thanks, Tagir. I agree these optimizations are worth doing. I have worked on similar toArray methods in other Collection implementations. I have a benchmark that is also a correctness test Collection/IteratorMicroBenchmark.java but I never added any Set implementations since I intentionally

RFR: jsr166 integration 2018-09

2018-09-26 Thread Martin Buchholz
https://cr.openjdk.java.net/~martin/webrevs/jdk/jsr166-integration/overview.html 8210971: Add exception handling methods to CompletionStage and CompletableFuture https://cr.openjdk.java.net/~martin/webrevs/jdk/jsr166-integration/exceptionally/index.html

Re: Collections.removeIf spec says nothing whether predicate may refer to collection

2018-09-24 Thread Martin Buchholz
We should have specified that the predicate is not permitted to access the collection at all, (but subclasses may support it). In practice, we took care during the implementation of the optimized removeIf methods to preserve invariants so that predicates that only access the collection for read

Re: RFR(s): 8210549: Runtime.exec: in closeDescriptors(), use FD_CLOEXEC instead of close()

2018-09-17 Thread Martin Buchholz
More concurrent discussion over in libc-alpha """system and popen fail in case of big application""" http://sourceware-org.1504.n7.nabble.com/system-and-popen-fail-in-case-of-big-application-tt534908.html#a537069 On Wed, Sep 12, 2018 at 6:17 AM, Martin Buchholz w

Re: RFR 8210787 : Object.wait(long, int) throws inappropriate IllegalArgumentException

2018-09-15 Thread Martin Buchholz
Looks good! On Fri, Sep 14, 2018 at 10:44 PM, Ivan Gerasimov wrote: > Hello! > > This is inspired by a recent fix for JDK- 8210004 (Thread.sleep(millis, > nanos) timeout returns early). > > Currently, Object.wait(Long.MAX_VALUE, 1) would throw > "IllegalArgumentException: timeout value is

Re: Re-iterate JDK-6194856: Zip Files lose ALL ownership and permissions of the files

2018-09-12 Thread Martin Buchholz
In principle I support making system specific extensions to the ZIP spec more supported, as other zip implementations do. There is long standing tension between Java trying to be highly portable and providing access to the data you need. It's already the case that some implementation bits are

Re: Runtime.exec : vfork() concerns and a fix proposal

2018-09-12 Thread Martin Buchholz
On Wed, Sep 12, 2018 at 8:16 AM, David Lloyd wrote: > Seems worthwhile though, given vfork's now-10-year-old obsolescence. > It looks like Linux is the only platform still using vfork for > ProcessImpl in OpenJDK. I'm fine with switching to posix_spawn on Linux as long as we don't reintroduce

Re: Runtime.exec : vfork() concerns and a fix proposal

2018-09-12 Thread Martin Buchholz
On Wed, Sep 12, 2018 at 7:33 AM, David Lloyd wrote: >> The child process is created using vfork(2) instead of fork(2) when [...] >> file_actions is NULL and the spawn-flags element of the attributes object >> pointed to by attrp does not contain POSIX_SPAWN_SETSIGMASK, >>

Re: Runtime.exec : vfork() concerns and a fix proposal

2018-09-12 Thread Martin Buchholz
I agree that your proposal makes the implementation more robust, so if you are willing to implement it and it doesn't cost too much, then I would support it. The current implementation doesn't seem to cause trouble in practice, or at least we don't see any bug reports. When you benchmark with

Re: RFR(s): 8210549: Runtime.exec: in closeDescriptors(), use FD_CLOEXEC instead of close()

2018-09-12 Thread Martin Buchholz
In 2018, we can assume everyone has implemented FD_CLOEXEC. Back in 1970, 20 file descriptors was enough for any program, but today they're effectively unlimited, so futilely closing _SC_OPEN_MAX descriptors has become unreasonable (as you say). OTOH, there is still no standard way of ensuring

Re: RFR(s): 8210549: Runtime.exec: in closeDescriptors(), use FD_CLOEXEC instead of close()

2018-09-12 Thread Martin Buchholz
On Wed, Sep 12, 2018 at 5:46 AM, Thomas Stüfe wrote: > On Wed, Sep 12, 2018 at 2:30 PM, Martin Buchholz wrote: > Btw, should I retry the readdir() on EINTR? If I read http://pubs.opengroup.org/onlinepubs/9699919799/functions/readdir.html correctly, then readdir should never fail with EINTR

Re: RFR(s): 8210549: Runtime.exec: in closeDescriptors(), use FD_CLOEXEC instead of close()

2018-09-12 Thread Martin Buchholz
The reference from_fd + 2 needs updating since it assumes two file descriptors have already been closed? On Tue, Sep 11, 2018 at 10:27 AM, Thomas Stüfe wrote: > Hi all, > > May I please have reviews for this small fix: > > Bug: https://bugs.openjdk.java.net/browse/JDK-8210549 > > patch (full):

Re: 8207690: Parsing API for classpath and similar path strings

2018-09-11 Thread Martin Buchholz
On Tue, Sep 11, 2018 at 9:04 AM, Stephen Colebourne wrote: > What I can say is that I think List is the better fit here, because: > - the result is not large If we're talking about classpath splitting ... At Google, we continue to struggle with classpaths that have on the order of 10,000

Re: Runtime.exec : vfork() concerns and a fix proposal

2018-09-11 Thread Martin Buchholz
https://sourceware.org/ml/libc-alpha/2018-09/msg00120. Coincidence?

Re: ByteArrayOutputStream should not have a new writeBytes method in Java 11

2018-09-11 Thread Martin Buchholz
It's an errorprone warning, for "easy" removal ... __IF__ you can run errorprone on the head sources. I think it's http://errorprone.info/bugpattern/MixedArrayDimensions On Tue, Sep 11, 2018 at 2:35 PM, Brian Burkhalter wrote: > > On Sep 11, 2018, at 1:23 PM, Stuart Marks wrote: > >>> 2. even

Re: RFR (XS) 8210347 : Combine subsequent calls to Set.contains() and Set.add()

2018-09-11 Thread Martin Buchholz
A meeting of the minds? http://hg.openjdk.java.net/jdk/jdk/rev/745ce8f5efc8 Looks good! On Tue, Sep 11, 2018 at 1:09 PM, Ivan Gerasimov wrote: > Hello! > > It's a pico-optimization. > > In a few places a call to Set.contains() can be merged with the following > Set.add(). > > BUGURL:

Re: Question about libjava/childproc.c

2018-09-09 Thread Martin Buchholz
On Sun, Sep 9, 2018 at 1:04 AM, Alan Bateman wrote: > On 09/09/2018 01:34, Martin Buchholz wrote: > > : > > See discussion in src/java.base/unix/native/libjava/ProcessImpl_md.c > > >> Later Oracle introduced something very similar with the jspawnhelper. >> But I

Re: Question about libjava/childproc.c

2018-09-08 Thread Martin Buchholz
On Wed, Sep 5, 2018 at 11:43 AM, Thomas Stüfe wrote: > On Wed, Sep 5, 2018 at 6:43 PM, Martin Buchholz > wrote: > > > So before the readdir loop we should set errno to 0. Then when readdir > > returns NULL, we should check whether errno is non-zero. > > > > Not

Re: [PATCH] Simplification of TreeMap

2018-09-05 Thread Martin Buchholz
Сергей, they say "If it ain't broke, don't fix it". Your implementation is indeed more maintainable, but TreeMap has been rather stable and is unlikely to see much maintenance! (unless value types makes it compelling). One of many problems with serialization is that cross-version compatibility

Re: Question about libjava/childproc.c

2018-09-05 Thread Martin Buchholz
On Wed, Sep 5, 2018 at 9:23 AM, Thomas Stüfe wrote: > > Since all this happens between vfork and exec we cannot do any decent > error handling here. Even what little we do is way outside the allowed > spec for vfork(). > Well we do have a fallback if closeDescriptors fails. But on closer

Re: Question about libjava/childproc.c

2018-09-05 Thread Martin Buchholz
Alan: Thomas seems to be suggesting setting the FD_CLOEXEC flag after fork but before exec, which is a slightly different idea. Thomas: This is an interesting idea. Historically the usual strategy was to close all the file descriptors explicitly, perhaps before FD_CLOEXEC was something we could

Re: RFR 8098798: Thread.join(ms) on Linux still affected by changes to the time-of-day clock

2018-09-04 Thread Martin Buchholz
On Tue, Sep 4, 2018 at 12:02 PM, Roger Riggs wrote: > >> One sharp corner is that wait(0) waits forever, and TimeUnit conversion >> truncates. You can probably avoid those problems via TimeUnit.timedWait. >> >> Not trivial since a long cannot hold the combined time of millis(long) > and nanos

Re: RFR 8210285 : CharsetDecoder/Encoder's constructor does not reject NaN

2018-09-03 Thread Martin Buchholz
Looks good to me. I would add a call to new MyDecoder(ascii, 0.5f, 1.5f) to make sure all calls to the constructor don't throw (because e.g. for ASCII we know the correct values are 1.0f). (In any case it feels like an API design mistake - the Charset itself should be the source of truth about

Re: RFR 8098798: Thread.join(ms) on Linux still affected by changes to the time-of-day clock

2018-08-29 Thread Martin Buchholz
Thanks for taking this on. Wait loops are notoriously hard to get right... One sharp corner is that wait(0) waits forever, and TimeUnit conversion truncates. You can probably avoid those problems via TimeUnit.timedWait. In code like this in j.u.c. we try to optimize away the call to nanoTime

Re: RFR: 8209937: Enhance java.io.Console - provide methods to query console width and height

2018-08-27 Thread Martin Buchholz
History: I was always a little disappointed that Console (understandably) didn't support the things you could do with subprocesses in Emacs, like implement shell buffers and otherwise have expect-style conversations with subprocesses. But console dimensions was never on my radar - doesn't mean

Re: RFR 8208715: Conversion of milliseconds to nanoseconds in UNIXProcess contains bug

2018-08-15 Thread Martin Buchholz
openjdk.java.net/~rriggs/webrev-timeout-8208715/ > > Thanks, Roger > > > > On 8/14/2018 6:14 PM, Martin Buchholz wrote: > > Looks good to me ... but ... > > my intent was that the self-interrupt was added in __addition__ to > interrupt by other thread, because there i

Re: RFR 8208715: Conversion of milliseconds to nanoseconds in UNIXProcess contains bug

2018-08-14 Thread Martin Buchholz
ebrev-timeout-8208715/index.html > > On 8/14/2018 1:22 PM, Martin Buchholz wrote: > > Thanks, Roger. I approve this version, although as always I have > comments. > > 520 private static native void waitForTimeoutInterruptibly( > 521 long handle, long timeout); >

Re: RFR 8208715: Conversion of milliseconds to nanoseconds in UNIXProcess contains bug

2018-08-14 Thread Martin Buchholz
18 at 9:48 AM, Roger Riggs wrote: > Hi Martin, > > I updated the webrev with the suggestions. > > On 8/14/2018 10:47 AM, Martin Buchholz wrote: > > hi Roger, > > 509 if (deadline <= 0) { 510 deadline = Long.MAX_VALUE; > 511 } >

Re: RFR 8208715: Conversion of milliseconds to nanoseconds in UNIXProcess contains bug

2018-08-14 Thread Martin Buchholz
hi Roger, 509 if (deadline <= 0) { 510 deadline = Long.MAX_VALUE; 511 } This must be wrong. Nanotime wraparound is normal in this sort of code. --- We ought to be able to delegate the fiddling with nanos to TimeUnit.timedWait. Does it work to simply call

Re: RFR 8209171 : Simplify Java implementation of Integer/Long.numberOfTrailingZeros()

2018-08-13 Thread Martin Buchholz
These variants are close to numberOfTrailingZeros_07 that I've already >> tested, though you did better by saving one arithmetic operation at the >> return line! >> >> I'll rerun the benchmarks. >> >> With kind regards, >> >> Ivan >> >>

Re: RFR 8209171 : Simplify Java implementation of Integer/Long.numberOfTrailingZeros()

2018-08-13 Thread Martin Buchholz
On Mon, Aug 13, 2018 at 10:10 AM, Ivan Gerasimov wrote: > Hi Martin! > > Good point about the command line flags, thanks! > > These variants are close to numberOfTrailingZeros_07 that I've already > tested, though you did better by saving one arithmetic operation at the > return line! > > Right.

Re: RFR 8209171 : Simplify Java implementation of Integer/Long.numberOfTrailingZeros()

2018-08-13 Thread Martin Buchholz
The number of plausible variants is astonishing! --- Your use of -client and -server is outdated, which explains why you get the same results for both (-client is ignored). I'm not sure what's blessed by hotspot team, but for C1 I use -XX:+TieredCompilation -XX:TieredStopAtLevel=1 and for C2 I

Re: RFR 8209171 : Simplify Java implementation of Integer/Long.numberOfTrailingZeros()

2018-08-12 Thread Martin Buchholz
Here's an example of a microbenchmark that uses multiple random inputs simulating various typical populations: https://github.com/google/caliper/blob/master/caliper-examples/src/main/java/examples/Utf8Benchmark.java

Re: RFR 8209171 : Simplify Java implementation of Integer/Long.numberOfTrailingZeros()

2018-08-12 Thread Martin Buchholz
h benchmark method to see a variety of inputs, perhaps in an array. On Sun, Aug 12, 2018 at 7:22 AM, Ivan Gerasimov wrote: > Hi Martin! > > On 8/11/18 5:54 PM, Martin Buchholz wrote: > > Hi Ivan, > > Oh the allure of bit-twiddling! > > Yes :) > > I'm skeptical th

Re: RFR 8209171 : Simplify Java implementation of Integer/Long.numberOfTrailingZeros()

2018-08-11 Thread Martin Buchholz
hese methods could be improved (but there are existing hotspot tests). nlz seems harder than ntz in the sense that for nlz "small ints" and random bits may have different optimal implementations. On Fri, Aug 10, 2018 at 7:03 PM, Ivan Gerasimov wrote: > Thanks Martin! > > On 8/9/18

Re: RFR 8209184: JDK8 ResourceBundle vulnerable to GC (fix included)

2018-08-10 Thread Martin Buchholz
retargeted bug to java.util:i18n, the right mailing list seems to be i18n-dev. On Fri, Aug 10, 2018 at 8:42 AM, Adam Farley8 wrote: > Hi All, > > This bug could be fixed by comparing the Class Loader with a blank > static volatile Object (defined outside the method scope) at the > end of the

Re: RFR 8209171 : Simplify Java implementation of Integer/Long.numberOfTrailingZeros()

2018-08-09 Thread Martin Buchholz
On Thu, Aug 9, 2018 at 5:27 PM, Ivan Gerasimov wrote: > I did not use the intrinsified variants of numberOfLeadingZeros in the > benchmark. > Oops! Should have looked more closely! Did you know about http://www.hackersdelight.org/hdcodetxt/ntz.c.txt

Re: RFR 8209171 : Simplify Java implementation of Integer/Long.numberOfTrailingZeros()

2018-08-09 Thread Martin Buchholz
On Thu, Aug 9, 2018 at 4:15 PM, Ivan Gerasimov wrote: > Hello! > > Integer.numberOfTrailingZeros() and Long.numberOfTrailingZeros() are > intrinsified by Hotspot, so the Java implementation of these is not too > important. > However, they still can be improved by a tiny bit :) > > Could you

Re: [12]RFR 8205399 : Set node color on pinned HashMap.TreeNode deletion

2018-08-08 Thread Martin Buchholz
Brent, thanks for writing a test. I would probably not have bothered. Or if I was ambitious, I might have written a whitebox test against all the red-black tree implementations in the core libs, verifying node color. +int hash; final? +static class Key implements Comparable {

Re: 8143850: retrofit ArrayDeque to implement List

2018-07-23 Thread Martin Buchholz
(As usual, I don't have enough time to devote to this at the moment) I sort of like the idea of adding all of those List methods to ArrayDeque, but it's __weird__ for java code to do that, and remove(int) is a problem when you have ArrayDeque, so it will probably end up rejected. --- Similarly,

Re: RFR 8207314 : Unnecessary reallocation when constructing WeakHashMap from a large Map

2018-07-23 Thread Martin Buchholz
is distracting in the source code and totally unimportant in practice, so I'd like to just not see it. On Fri, Jul 20, 2018 at 6:42 PM, Ivan Gerasimov wrote: > Hi Martin, > > Thank you for looking into this! Please see the comments inline. > > On 7/15/18 9:14 AM, Martin Buchholz wro

Re: [12] 8206403: ByteArrayOutputStream hugeCapacity method can return invalid capacity

2018-07-23 Thread Martin Buchholz
@oracle.com> wrote: > > On Jul 23, 2018, at 5:26 PM, Martin Buchholz wrote: > > I'm the author of most of the MAX_ARRAY_SIZE code in the jdk. > We should be as consistent as we can given the history. > > > Thanks for the history lesson. > > It all looks pretty simila

Re: [12] 8206403: ByteArrayOutputStream hugeCapacity method can return invalid capacity

2018-07-23 Thread Martin Buchholz
I'm the author of most of the MAX_ARRAY_SIZE code in the jdk. We should be as consistent as we can given the history. It all looks pretty similar. --- +if (minCapacity < 0) // overflow +throw new OutOfMemoryError("Memory capacity overflow: " ++ minCapacity);

Re: [RFC] StringBuilder.toCharArray

2018-07-18 Thread Martin Buchholz
You can already copy a StringBuilder's contents into a String or another StringBuilder. What would Josh say? "Doesn't pull its weight." ?! On Wed, Jul 18, 2018 at 7:29 AM, Isaac Levy wrote: > Is there any interest in a toCharArray method for StringBuilder? I'm > unable to to make a bug for it.

Re: Adding new IBM extended charsets

2018-07-17 Thread Martin Buchholz
History: I recall 15 years ago wondering why no one from IBM was maintaining the IBM charsets in the jdk. Today anything but UTF-* is increasingly legacy, so the case for including them is weaker than back then. Probably today the IBM charsets are best maintained outside the jdk sources as a

Re: RFR 8207314 : Unnecessary reallocation when constructing WeakHashMap from a large Map

2018-07-15 Thread Martin Buchholz
Hi Ivan, Here are some thoughts while looking at this: --- WeakHashMap promises to have similar "capacity" handling to HashMap, but implementations (and doc?) seem more different than necessary (diverged over time?), and there don't seem to be any tests. HashMap seems to deal with this problem

Re: ArrayList.ensureCapacity should expand the default capacity empty array

2018-07-09 Thread Martin Buchholz
Thanks Alex, I filed https://bugs.openjdk.java.net/browse/JDK-8206945 I've had similar thoughts for years, but none have ended up in the actual code. I haven't seem the approach you used in your reallocate. On Thu, Jul 5, 2018 at 10:04 AM, Alex Foster wrote: > Hi, > > > I think that

RFR: 8206863: A closed JarVerifier.VerifierStream should throw IOException

2018-07-09 Thread Martin Buchholz
Here's another fix from Tobias and myself: 8206863: A closed JarVerifier.VerifierStream should throw IOException http://cr.openjdk.java.net/~martin/webrevs/jdk/JarVerifier-stream-closed/ https://bugs.openjdk.java.net/browse/JDK-8206863

Re: RFR JDK-8206389: JarEntry.setCreation/LastAccessTime without setLastModifiedTime causes Invalid CEN header

2018-07-07 Thread Martin Buchholz
OK, looks good! I would add something to check() to make sure that e.g. atime == null iff ze.getLastAccessTime() == null The zip time stuff is surprisingly messy. On Fri, Jul 6, 2018 at 8:50 PM, Xueming Shen wrote: > On 7/6/18, 5:43 PM, Martin Buchholz wrote: > > I would use

Re: RFR JDK-8206389: JarEntry.setCreation/LastAccessTime without setLastModifiedTime causes Invalid CEN header

2018-07-06 Thread Martin Buchholz
I would use different timestamps for the 4 possible times used in this test, if only to make it clearer which value comes from where. +// ze.setLastModifiedTime(now);+ ze.setTime(now.toMillis()); setTime only sets the DOS time? Which only has a granularity of 2 seconds? If so, how

RFR: jsr166 integration 2018-07

2018-07-06 Thread Martin Buchholz
http://cr.openjdk.java.net/~martin/webrevs/jdk/jsr166-integration/overview.html Some smallish changes; all of them already seen review elsewhere. 8206123: ArrayDeque created with default constructor can only hold 15 elements

Re: Useless null check in HashMap.merge()

2018-07-05 Thread Martin Buchholz
The null check will be removed as part of the next jsr166 integration. http://cr.openjdk.java.net/~martin/webrevs/jdk/jsr166-integration/miscellaneous/index.html On Wed, Jul 4, 2018 at 2:42 AM, Zheka Kozlov wrote: > I noticed dead code in java.util.HashMap.merge(): > > public V merge(K key, V

Re: RFR 8206123 : ArrayDeque created with default constructor can only hold 15 elements

2018-07-04 Thread Martin Buchholz
On Tue, Jul 3, 2018 at 9:31 PM, Ivan Gerasimov wrote: > Hi Martin! > > Why did you exclude the case of zero initial capacity? > 96 int initialCapacity = rnd.nextInt(1, 20); > > Well spotted - I was just being lazy - ArrayDeque doesn't handle the case of zero specially. public

Re: RFR 8206123 : ArrayDeque created with default constructor can only hold 15 elements

2018-07-03 Thread Martin Buchholz
OK, this thread is officially upgraded to a RFR. http://cr.openjdk.java.net/~martin/webrevs/jdk/jsr166-integration/ArrayDeque-capacity/index.html On Tue, Jul 3, 2018 at 11:04 AM, Paul Sandoz wrote: > > > On Jul 3, 2018, at 10:42 AM, Martin Buchholz wrote: > > > On Tue, Jul

Re: RFR 8206123 : ArrayDeque created with default constructor can only hold 15 elements

2018-07-03 Thread Martin Buchholz
On Tue, Jul 3, 2018 at 9:53 AM, Paul Sandoz wrote: > Looks good. Where do you propose to place the test in the OpenJDK repo? > http://cr.openjdk.java.net/~martin/webrevs/jdk/jsr166-integration/overview.html I hadn't gotten around to a RFR - the test is slightly modified from my initial post.

Re: ArrayDeque and List

2018-06-30 Thread Martin Buchholz
https://openjdk.markmail.org/search/?q=8143850#query:8143850%20list%3Anet.java.openjdk.core-libs-dev+page:1+state:facets On Sat, Jun 30, 2018 at 12:53 PM, Jonas Konrad wrote: > Hey, > > The introduction of ArrayDeque in Java 1.6 has basically replaced > LinkedList as the "default"

Re: RFR 8206123 : ArrayDeque created with default constructor can only hold 15 elements

2018-06-30 Thread Martin Buchholz
30 Jun 2018 17:07:46 - @@ -0,0 +1,125 @@ +/* + * Written by Martin Buchholz with assistance from members of JCP + * JSR-166 Expert Group and released to the public domain, as + * explained at http://creativecommons.org/publicdomain/zero/1.0/ + */ + +/* + * @test + * @modules java.base

Re: ImmutableCollections.MapN optimisation ?

2018-06-30 Thread Martin Buchholz
On Sat, Jun 30, 2018 at 9:05 AM, John Rose wrote: > On Jun 30, 2018, at 8:39 AM, Martin Buchholz wrote: > > > >> On Sat, Jun 30, 2018 at 3:54 AM, Remi Forax wrote: > >> > >> The other problem is more subtle, get() uses probe() and probe() uses an >

Re: ImmutableCollections.MapN optimisation ?

2018-06-30 Thread Martin Buchholz
On Sat, Jun 30, 2018 at 3:54 AM, Remi Forax wrote: > The other problem is more subtle, get() uses probe() and probe() uses an > infinite loop and not a counted loop, the result is that c2 generates lot > of assembly codes for probe() as a consequence it doesn't inline probe() in > get() making

Re: 8143850: retrofit ArrayDeque to implement List

2018-06-29 Thread Martin Buchholz
On Thu, Jun 28, 2018 at 11:42 PM, Alex Foster wrote: > Another question is whether or not it should allow null elements. > ArrayDeque currently doesn't but I think it would be useful to support > them, which makes having an asList() view or sharing code harder. > JDK has moved away from null

Re: 8143850: retrofit ArrayDeque to implement List

2018-06-28 Thread Martin Buchholz
Thanks, Alex. This has been on my todo list for a long time and I'm more overcommitted than ever. The difficult part is designing of the interface, but then there are many tests to write... I sort of liked ArrayDeque.asList() because at least then the API surface to be added is small.

Re: [PATCH] AbstractStringBuilder.append()

2018-06-28 Thread Martin Buchholz
On Thu, Jun 28, 2018 at 11:59 AM, Isaac Levy wrote: > And can't remove append(StringBuffer) because of binary compatibility? > That seems right. --- Also, the JIT can optijmize away any instanceof checks after inining when it sees append(stringBuilder). And any optimizations here are far

Re: [PATCH] AbstractStringBuilder.append()

2018-06-27 Thread Martin Buchholz
I'm fairly sure the append(StringBuilder) overloads were left out intentionally. On Wed, Jun 27, 2018 at 8:57 PM, Isaac Levy wrote: > AbstractStringBuilder already has append(). This patch > adds append(). > > The new method improves parity between the two classes. In addition, > combining

Re: RFR(XXS): 8205916: [test] Fix jdk/tools/launcher/RunpathTest to handle both, RPATH and RUNPATH

2018-06-27 Thread Martin Buchholz
Looks good to me! On Wed, Jun 27, 2018 at 3:26 AM, Volker Simonis wrote: > Hi, > > can I please have a review for the following tiny test fix (I'm > actually not sure which would be the appropriate mailing list for this > fix so please redirect if necessary): > >

Re: RFR: 8205184: Delegating Iterator implementations that don't delegate forEachRemaining()

2018-06-25 Thread Martin Buchholz
Committed.

Re: RFR: 8205184: Delegating Iterator implementations that don't delegate forEachRemaining()

2018-06-25 Thread Martin Buchholz
On Mon, Jun 25, 2018 at 11:17 AM, Paul Sandoz wrote: > Looks good. Sometimes in these cases i reach for proxy rather than > explicit code. It tends to be more robust to certain changes at the expense > of reflective complexity. > > Hmmm ... Never done that in a test before. > You could

Re: RFR: jsr166 integration 2018-06

2018-06-25 Thread Martin Buchholz
Committed except for http://cr.openjdk.java.net/~martin/webrevs/jdk/jsr166-integration/overview.html 8203662: remove increment of modCount from ArrayList and Vector replaceAll() http://cr.openjdk.java.net/~martin/webrevs/jdk/jsr166-integration/replaceAll/index.html

Re: RFR: jsr166 integration 2018-06

2018-06-22 Thread Martin Buchholz
On Fri, Jun 22, 2018 at 10:28 AM, Paul Sandoz wrote: > > > --- > > We've been unable to reach agreement on 8203662 - it remains in limbo. > > > > 8203662: remove increment of modCount from ArrayList and Vector > replaceAll() > > http://cr.openjdk.java.net/~martin/webrevs/jdk/jsr166- >

<    1   2   3   4   5   6   7   8   9   10   >