Re: RFR: 4511638: Double.toString(double) sometimes produces incorrect results [v15]
On Wed, 1 Jun 2022 10:37:23 GMT, Raffaello Giulietti wrote: >> Hello, >> >> here's a PR for a patch submitted on March 2020 >> [1](https://cr.openjdk.java.net/~bpb/4511638/webrev.04/) when Mercurial was >> a thing. >> >> The patch has been edited to adhere to OpenJDK code conventions about >> multi-line (block) comments. Nothing in the code proper has changed, except >> for the addition of redundant but clarifying parentheses in some expressions. >> >> >> Greetings >> Raffaello > > Raffaello Giulietti has updated the pull request incrementally with one > additional commit since the last revision: > > 4511638: Double.toString(double) sometimes produces incorrect results Hallelujah! - PR: https://git.openjdk.java.net/jdk/pull/3402
Re: Java floating point and StrictMath improvement?
On 16/03/2022 09:56, Raffaello Giulietti wrote: "Computer scientists of the world, unite! Let's get rid of binary floating-point arithmetic in Java, after 27 years of honorable service! Let's adopt decimal floating-point arithmetic, where 1 / 3 + 1 / 3 is still different from 2 / 3, but who cares? At least we have 0.1 * 0.1 = 0.01, as Mathematics commands!" Just kidding... More seriously, let's stop this useless discussion. Raffaello, the problem initially apparent here is that the person posting these rants is ignorant of some very straightforward knowledge about numbers and numeric representation, both mathematical and via fixed or floating point approximation. Surprising, perhaps in someone who claims to be a programmer, since it does not even require degree level study in mathematics to acquire such knowledge. A competent amateur or a degree student in a science or engineering field can easily learn what is at stake and note the misconceptions the rants are based on. However, on top of basic ignorance, it takes real wilfulness and stupidity -- not to mention a certain measure of narcissism -- for someone: to suggest that the various people, like yourself, who clearly do understand the relevant mathematical facts and have replied at length to explain those misconceptions, cannot know what they are talking about; and further to demand that some higher authority turn up as an act of duty, like a latter-day 8th cavalry, to sanction their incoherent proposals. Best not to feed someone like this; trolls are greedy beasts and grow to disproportionate size the more you feed them. regards, Andrew Dinn ---
Re: RFR: 8268231: Aarch64: Use ldp in intrinsics for String.compareTo [v6]
On Fri, 17 Sep 2021 07:13:24 GMT, Wu Yan wrote: >> It's fine. I don't think it'll affect any real programs, so it's rather >> pointless. I don't know if that's any reason not to approve it. > > Andrew, can you help us to approve this? I agree with Andrew Haley that this patch is not going to make an improvement for anything but a very small number of applications. Processing of strings over a few 10s of bytes is rare. On the other hand the doesn't seem to cause any performance drop for the much more common case of processing short strings. so it does no harm. Also, the new and old code are much the same in terms of complexity so that is no reason to prefer one over the other. The only real concern I have is that any change involves the risk of error and the ratio of cases that might benefit to cases that might suffer from an error is very low. I don't think that's a reason to avoid pushing this patch upstream but it does suggest that we should not backport it. - PR: https://git.openjdk.java.net/jdk/pull/4722
Re: RFR: 8188044: We need Math.unsignedMultiplyHigh [v2]
On 02/07/2021 16:30, Raffaello Giulietti wrote:
FWIW, adinn's branchless code together with
PR https://git.openjdk.java.net/jdk/pull/4660
make both methods less vulnerable to timing attacks.
That was indeed the point of the change. However, I doubt the difference
in timing is going to be significant given Andrew's micro-benchmark results.
regards,
Andrew Dinn
---
Red Hat Distinguished Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill
Re: RFR: 8188044: We need Math.unsignedMultiplyHigh [v2]
On Fri, 2 Jul 2021 09:39:46 GMT, Andrew Haley wrote: >> src/java.base/share/classes/java/lang/Math.java line 1211: >> >>> 1209: long z1 = t >>> 32; >>> 1210: >>> 1211: return x1 * y1 + z1 + (z0 >>> 32); >> >> Suggestion: >> >> long result = Math.multiplyHigh(x, y); >> if (x < 0) result += y; >> if (y < 0) result += x; >> return result; > > This is just subtracting the 2's-complement offset. I guess the idea, longer > term, is that this be an intrinsic anyway, but if you do > `unsignedMultiplyHigh` this way you'll utilize the existing `multiplyHigh` > intrinsic on all platforms that have it. You can also do that branchlessly which might prove better long result = Math.multiplyHigh(x, y); result += (y & (x >> 63)); result += (x & (y >> 63)); return result; - PR: https://git.openjdk.java.net/jdk/pull/4644
Re: RFR: 8269384: Determine native byte order for StringUTF16 via ByteOrder
On Fri, 25 Jun 2021 13:40:54 GMT, Yi Yang wrote: > Prefer using ByteOrder to compute byte order for StringUTF16 to determining > byte order by native method StringUTF16.isBigEndian. Hi Yi Yang, This is more complex than it seems. The general policy is not to change boot order if at all possible and for good reason. there are many ways it can cause unexpected consequences. Regarding this specific case, UnsafeConstants exists in order to allow class Unsafe to define a few final fields derived from runtime environment/hardware parameters provided as final static *primitive* fields of UnsafeConstants. The relevant fields in UnsafeConstants are injected by the JVM after they have been initialized to dummy values by the class's method. So, the purpose of the class is simply to pre-cache these runtime/hardware values as Java state, avoiding the need for Unsafe to call out to the JVM using native code. Before UnsafeConstants was defined these constants used to be retrieved using native callouts. What you are asking for is to make String use the same pre-cache as Unsafe i.e. to expand the set of classes which depend on UnsafeConstants to include String. While that might work as currently defined it is not clear that it will always continue to work in the future. That's because String is a rather special case, much like a primitive class. Instances of String are created to represent literal terms in the language. Note that these String terms are themselves *constants*. UnsafeConstants is not meant to have any other uses except to cache the runtime-specific/hardware constants used by Unsafe. So, it ought not to depend on other classes. However, it is also important that no other classes depend on it and that includes String. Now imagine someone were to update UnsafeConstants to include an injected String constant -- say, the current setting for LC_LANG, or the processor CPU name or some other legitimate text value derived form the runtime or hardware. Your change would cause a problem because initialization of UnsafeConstants would require String already to be initialized. Of course, neither of those examples is a likely candidate for inclusion in UnsafeConstants but it is hard to say whether other more realistic cases might arise in future. - PR: https://git.openjdk.java.net/jdk/pull/4596
Re: RFR: 8265768 [aarch64] Use glibc libm impl for dlog, dlog10, dexp iff 2.29 or greater on AArch64.
On Thu, 15 Apr 2021 08:33:47 GMT, gregcawthorne wrote: > Glibc 2.29 onwards provides optimised versions of log,log10,exp. > These functions have an accuracy of 0.9ulp or better in glibc > 2.29. > > Therefore this patch adds code to parse, store and check > the runtime glibcs version in os_linux.cpp/hpp. > This is then used to select the glibcs implementation of > log, log10, exp at runtime for c1 and c2, iff we have > glibc 2.29 or greater. > > This will ensure OpenJDK can benefit from future improvements > to glibc. > > Glibc adheres to the ieee754 standard, unless stated otherwise > in its spec. > > As there are no stated exceptions in the current glibc spec > for dlog, dlog10 and dexp, we can assume they currently follow > ieee754 (which testing confirms). As such, future version of > glibc are unlikely to lose this compliance with ieee754 in > future. > > W.r.t performance this patch sees ~15-30% performance improvements for > log and log10, with ~50-80% performance improvements for exp for the > common input ranged (which output real numbers). However for the NaN > and inf output ranges we see a slow down of up to a factor of 2 for > some functions and architectures. > > Due to this being the uncommon case we assert that this is a > worthwhile tradeoff. > [ One thing: Java uses the term "semi-monotonic" to > mean "whenever the mathematical function is non-decreasing, so is > the floating-point approximation, likewise, whenever the > mathematical function is non-increasing, so is the floating-point > approximation." I don't really understand what distinction means. ] I believe this is to allow for the fact that the function is continuous and the floating-point approximation is discrete. Let F be the actual function and f the floating point approximation. Assume we have two successive floating point values x, x' and, without loss of generality, F(x) <= F(x'). What are the circumstances under which we require f(x) =< f(x')? Semi-monotonicity says that is only needed when F is non-decreasing on the interval [x, x']. Expressed more precisely, the condition that F is non-decreasing is for all y such that x =< y =< x' : F(x) <= F(y) <= F(x'). In other words: if the graph only ever stays level or increases across the interval [x, x'] then we must have f(x) =< f(x') If the graph wiggles *up* and *down* across the interval [x, x'] we can allow f(x) > f(x'). - PR: https://git.openjdk.java.net/jdk/pull/3510
Re: RFR: 8200559: Java agents doing instrumentation need a means to define auxiliary classes [v2]
On Fri, 16 Apr 2021 20:30:15 GMT, Rafael Winterhalter wrote: >> To allow agents the definition of auxiliary classes, an API is needed to >> allow this. Currently, this is often achieved by using `sun.misc.Unsafe` or >> `jdk.internal.misc.Unsafe` ever since the `defineClass` method was removed >> from `sun.misc.Unsafe`. > > Rafael Winterhalter has refreshed the contents of this pull request, and > previous commits have been removed. The incremental views will show > differences compared to the previous content of the PR. Sorry to be late to this party. I've been wanting to read this thread for a while but have bene too busy up to now. I have just a few comments I too was party to the discussions about agent capabilities and recall well the decision to gradually impose restrictions, the first one being to control dynamic agent loading. I was happy to accept that general decision and the specific one to limit the opportunity for an agent self-hoisting into the current JVM. However, a key part of the plan to move forward on restrictions was to provide an override switch. I'd very much like to see that retained as an option. I know that in some uses self-hoisting is much preferable to having to install the agent on the command line and I'd expect he same to be true for any other capabilities for which restrictions were adopted. Although it is true -- as Ron said - -that configuring -javaagent on the command line is always /possible/ there are actually many scenarios for agent use where deployment of an agent after startup is pragmatically much more desirable. An obvious use is trouble shooting, where you only want an agent in place when something goes wrong. That turns out to be critical to solving some seriously difficult support cases. The interesting use cases also fall under testing where self-hoisting of a test agent by a test framework can result in an enormous simplification of test configuration. Usage of Byteman for testing went through the roof with this capability in place. Never underestimate the degree to which even the most minimal configuration complexity puts off Enterprise java developers when it is multiplied by the test suite size of a large project. So, likewise with other restrictions on behaviour, I'm very happy to see them put in place for dynamically hoisted agents so long as there is still a command line override along the lines of the agent attach property that allows a dynamic agent to do all that a command line agent can. One other thing I'd like to correct is a point made in the discussion about agent code residing in the system loader. It is true that the main agent class gets loaded by the system loader. However, it is perfectly possible to ensure that all the rest of the agent code is loaded by the bootstrap loader. A Main class can add the agent jar to the bootstrap path and then load and use reflection to invoke an effective main routine on a bootstrap loaded SubMain class. Byteman uses this trick on request in order to allow it to instrument bootstrap classes. Because all the Byteman classes except for the original Main shell class are loaded by the bootstrap loader Byteman can safely inject references to the Byteman rule engine and Byteman exception classes into bootstrap code. - PR: https://git.openjdk.java.net/jdk/pull/3546
Re: Conflict API definitions of Math.pow(x, 0.5) and Math.sqrt(x) for x={-0.0, Double.NEGATIVE_INFINITY}
On 12/04/2021 07:51, jiefu(傅杰) wrote:
I think most people will be confused by these rules because from the
view of mathematics, Math.pow(x, 0.5) should be equal to
Math.sqrt(x).
This is already a confused situation from the point of view of
mathematics. Consider these two expressions:
Math.sqrt(-0.0) * Math.sqrt(-0.0)
Math.pow(-0.0, 0.5) * Math.pow(-0.0, 0.5)
It doesn't matter whether the functions sqrt and pow compute -0.0 or 0.0
as the value here. Either result will fail to satisfy the equality
f(x) * f(x) == x
The problem is that computation has already diverged from mathematical
expectation by introducing the value -0.0. So, Java (and other
languages) have to make up a rule here.
So why Java creates conflict special rules for them? Is it possible
to let Math.pow(-0.0, 0.5) = -0.0 and
Math.pow(Double.NEGATIVE_INFINITY, 0.5) = NaN also be allowed?
It might well match expectations better if both functions were to
generate the same value here. However, expectations have already been
set by libc:
$ cat > sqrt.c << END
#include
#include
int main(int argc, char **argv) {
printf("sqrt(-0.F) = %f\n", sqrt(-0.F));
printf("pow(-0.F, 0.5) = %f\n", pow(-0.F, 0.5));
}
END
$ make sqrt
cc sqrt.c -o sqrt
$ ./sqrt
sqrt(-0.F) = -0.00
pow(-0.F, 0.5) = 0.00
I have no idea why these specific results were made up for C but Java
really ought to follow them.
regards,
Andrew Dinn
---
Red Hat Distinguished Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill
Re: RFR: 8264896: Remove redundant '& 0xFF' from int-to-byte cast
On 08/04/2021 10:11, Daniel Fuchs wrote:
On Thu, 8 Apr 2021 08:54:35 GMT, Sebastian Stenzel
wrote:
When we do
byte b1 = (byte) (value & 0xFF);
we keep from int only 1 lower byte and exactly the same can be achieved with
plain cast. See the test below:
public class Main {
public static void main(String[] args) throws Exception {
IntStream.range(Integer.MIN_VALUE, Integer.MAX_VALUE).forEach(value -> {
byte b1 = (byte) (value & 0xFF);
byte b2 = (byte) value;
if (b1 != b2) {
throw new RuntimeException("" + value);
}
});
}
Also tier1 and tier2 are both OK.
I don't think these masks have been added because they are _required_ but
rather because they explicitly show the intention: You can immediately see that
losing the MSBs of an int during the cast is _not_ just an error.
I agree with Sebastian. I believe the original code was clearer.
The same applies for the (value >>> 0) expressions that this patch also
removes. The reason for keeping them is to emphasize the continuity with
the (value >>> 24), (value >>> 16), etc cases that precede them.
Since none of this will make any difference to performance I think it is
better to keep this code as is.
regards,
Andrew Dinn
---
Red Hat Distinguished Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill
Re: RFR: 8173585: Intrinsify StringLatin1.indexOf(char)
On Tue, 22 Sep 2020 02:31:14 GMT, Vladimir Kozlov wrote: >>> >Can you explain where this restricted effect is documented? >> >>> Certainly! I’ve found that determining the capability of the CPU and >>> whether to enable AVX2 support if the chip >>> supports it is mostly controlled in: vm_version_x86.cpp specifically: >>> get_processor_features and in >>> generate_get_cpu_info. >> >> Yes, I can see what the code does. I was asking where the cpu behaviour is >> documented independent of the code. >> >>> In order to test the patch comprehensively I had to track down an Intel >>> Core i7 (I7-9750H) processor which the >>> aforementioned code permitted AVX2 instructions for (maybe this is an error >>> and it should not be enabled for this >>> processor though) as most of the infrastructure I personally use here at >>> AWS runs on Intel Xeon processors - I also >>> tested on a E5-2680 which the JVM does not enable AVX2 for. >> >> 'maybe'? The documentation Andrew provided mentioned Xeon E5 v3 which I >> believe is a Skylake design. However, the code >> I pointed you at in vm_version_x86 which claims to detect 'early Skylake' >> designs is only disabling AVX512 support. It >> still enables AVX2. Similarly, the code that generates machine code to check >> the processor capabilities has a special >> check if use_evex is set (i.e. AVX3 is requested) which disables AVX512 for >> Skylake but does not disable AVX2 support. >>> However, this is just the Intel side of things. When it comes to AMD I read >>> that the AMD Zen 2 architecture, of which >>> the current flagship: Threadripper 3990X, is based, is able to support AVX2 >>> without the frequency scaling which >>> some/all(?) of the Intel chips incur. I personally don’t have access to one >>> of these chips so I cannot confirm how it >>> is classified in the JVM. >> >> Well, it would be good to know where you read that and to see if that >> confirms thar the code is avoiding the issue >> Andrew raised. >>> Also, I found when investigating this that there is actually a JVM flag >>> which can be used to control what level of AVX >>> is enabled: -XX:UseAVX=version. >> >> Yes, indeed. However, what I am trying to understand is whether the current >> code is bypassing the problem Andrew >> brought up in the cases where that problem actually exists. It doesn't look >> like it so far given that the problem >> applies to AVX2 and only AVX512 support is being disabled and, even then >> only for some (Skylake) processors. Without >> some clear documentation of what processors suffer from this power surge >> problem it will not be possible to decide >> whether this patch is doing the right thing or not. > > Based on comment by @jatin-bhateja (Intel) frequency level switchover pointed > by @theRealAph is sensitive to vector > size https://github.com/openjdk/jdk/pull/144#issuecomment-692044896 > > By keeping vector size less or equal to 32 bytes we should avoid it. And as I > can see this intrinsic code is using 32 > bytes (chars) and 16 bytes vectors: `pbroadcastb(vec1, vec1, > Assembler::AVX_256bit);` > Also we never had issues with AVX2. only with AVX512 regarding performance > hit: > https://bugs.openjdk.java.net/browse/JDK-8221092 > > I would like to see performance numbers for for all values of UseAVX flag : > 0, 1, 2, 3 > > The usage is guarded UseSSE42Intrinsics in UseSSE42Intrinsics predicate in > .ad file. Make sure to test with UseAVX=0 to > make sure that some avx instructions are not mixed into non avx code. And > also with UseSSE=2 (for example) to make sure > shared code correctly recognize that intrinsics is not supported. @vnkozlov @mknjc @jatin-bhateja Thanks for providing the relevant details. I'm now quite content that this patch avoids any potential frequency scaling problem. I'm also glad that an explanation of why this is so is now available -- although it's not perfect that we are relying on a stackoverflow post for the full details. - PR: https://git.openjdk.java.net/jdk/pull/71
Re: RFR: 8173585: Intrinsify StringLatin1.indexOf(char)
On Fri, 18 Sep 2020 23:11:46 GMT, Jason Tatton wrote: > >Can you explain where this restricted effect is documented? > Certainly! I’ve found that determining the capability of the CPU and whether > to enable AVX2 support if the chip > supports it is mostly controlled in: vm_version_x86.cpp specifically: > get_processor_features and in > generate_get_cpu_info. Yes, I can see what the code does. I was asking where the cpu behaviour is documented independent of the code. > In order to test the patch comprehensively I had to track down an Intel Core > i7 (I7-9750H) processor which the > aforementioned code permitted AVX2 instructions for (maybe this is an error > and it should not be enabled for this > processor though) as most of the infrastructure I personally use here at AWS > runs on Intel Xeon processors - I also > tested on a E5-2680 which the JVM does not enable AVX2 for. 'maybe'? The documentation Andrew provided mentioned Xeon E5 v3 which I believe is a Skylake design. However, the code I pointed you at in vm_version_x86 which claims to detect 'early Skylake' designs is only disabling AVX512 support. It still enables AVX2. Similarly, the code generates machine code to check the processor capabilities has a special check if use_evex is set (i.e. AVX3 is requested) for Skylake which disables AVX512 but does nto disable AVX2 support. However, this is just the Intel side of things. When it comes to AMD I read that the AMD Zen 2 architecture, of which the current flagship: Threadripper 3990X, is based, is able to support AVX2 without the frequency scaling which some/all(?) of the Intel chips incur. I personally don’t have access to one of these chips so I cannot confirm how it is classified in the JVM. Also, I found when investigating this that there is actually a JVM flag which can be used to control what level of AVX is enabled: -XX:UseAVX=version. - PR: https://git.openjdk.java.net/jdk/pull/71
Re: RFR: 8173585: Intrinsify StringLatin1.indexOf(char)
On Fri, 18 Sep 2020 15:55:52 GMT, Jason Tatton wrote: >> Hi Andrew, >> >> Thanks for coming back to me. Looking on the github >> [PR](https://github.com/openjdk/jdk/pull/71) nobody is tagged as a >> reviewer for this (perhaps this is a feature which is not being used). >>> That's >>> because what is actually missing is the justification he asked for. As >>> Andrew pointed out the change is simple but the reason for implementing >>> it is not. >> >> There are two separate things here: >> 1). Justification for the change itself: >> -The objective and justification for this patch is to bring the performance >> of StringLatin1 indexOf(char) in line with >> StringUTF16 indexOf(char) for x86 and ARM64. This solves the problem as >> raised in >> [JDK-8173585](https://bugs.openjdk.java.net/browse/JDK-8173585), and also >> on the [mailing >> >> list](http://mail.openjdk.java.net/pipermail/jdk9-dev/2017-January/005539.html). >> >> 2). Discussion around future enhancements - concerning potential use of >> AVX-512 instructions and a more optimal >> implementation for short strings. >> -These would be separate JBS's I'm not advocating for/against this, they are >> just ideas separate from this JBS. > > The AVX2 code path represents approximately 1/6th of the patch (1/7th > including the infrastructure code around this). > I don’t think we should discard the entire patch because 1/6th of the code > may have unintended consequences. This is > especially the case when the rest of the code has been benchmarked, with > certainty, to show the desired performance > improvement has been achieved. Additionally, I do not see how those > unintended consequences will ever be realised > because the JVM has knowledge of the AVX capability of the chip it’s running > on and disables the AVX2 code path for > chips which suffer from the performance degradation which has been outlined > in this discussion. Thus protecting us from > unintended consequences. Unless we are asserting that this mechanism to > globally control the use of AVX2 instructions > is broken or otherwise non functional I see no reason to remove the AVX2 > code. And to be consistent we would really > need to look at removing all instances of AVX2 code in the JVM (of which > there is quite a lot). As I see it there are > three ways forward: 1. Accept the patch as is. 2. Modify the patch to > remove the AVX code path for x86, and/or any > other modifications needed. 3. Discard the patch entirely. At this point I > am in favour of approach 1 but happy to > accept 2 if advised that this is the right thing to do. "the JVM has knowledge of the AVX capability of the chip it’s running on and disables the AVX2 code path for chips which suffer from the performance degradation which has been outlined in this discussion" Does it? The white paper Andrew cited doesn't mention this as being specific to only some chips that implement AVX2. Can you explain where this restricted effect is documented? Also, I assume you are referring to the code in vm_version_x86.cpp with this comment // Don't use AVX-512 on older Skylakes unless explicitly requested is that correct? - PR: https://git.openjdk.java.net/jdk/pull/71
Re: RFR: 8173585: Intrinsify StringLatin1.indexOf(char)
On 18/09/2020 12:06, Jason Tatton wrote:
> There are two separate things here:
> 1). Justification for the change itself:
> -The objective and justification for this patch is to bring the performance
> of StringLatin1 indexOf(char) in
> line with StringUTF16 indexOf(char) for x86 and ARM64. This solves the
> problem as raised in
> [JDK-8173585](https://bugs.openjdk.java.net/browse/JDK-8173585), and also on
> the [mailing
>
> list](http://mail.openjdk.java.net/pipermail/jdk9-dev/2017-January/005539.html).
> 2). Discussion around future enhancements - concerning potential use of
> AVX-512 instructions and a more optimal
> implementation for short strings.
> -These would be separate JBS's I'm not advocating for/against this, they are
> just ideas separate from this JBS.
I don't agree that these two things are separable. Andrew's point
applies to both.
In the first case the problem is that the 'evidence' we have does not
testify to the possibility Andrew outlines. Both code examples used to
justify the idea StringLatin1.indexOf(char) will perform 'better' with
an AVX-based intrinsic are micro-benchmarks that do a lot of intensive
String manipulation and nothing else i.e. they won't get hit by the
possible cost of ramping up power for AVX because they make extensive
use of AVX and then stop. That's very unlikely to happen in a real world
case. So, the fact that this change removes the disparity seen with
these benchmarks is still not evidence for a general improvement.
So, I don't (yet) see a reason to make this change and the possibility
still stands that adopting this change may end up making most code that
uses StringLatin1.indexOf(char) worse.
It might be a good idea to consider finding some way to test whether the
cost Andrew has highlighted makes a difference before committing this
change. I know the same argument might might be raised aginst the
existing intrinsics but surely that's an a fortiori argument for not
proceeding.
regards,
Andrew Dinn
---
Red Hat Distinguished Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill
Re: RFR: 8173585: Intrinsify StringLatin1.indexOf(char)
On 17/09/2020 19:22, Jason Tatton wrote:
> This patch is just missing a couple of reviewers... Please can someone step
> forward?
>
> I think it's a fairly straightforward change.
I believe you got a review from Andrew Haley -- it was quoted in your
follow-up from which I selected the above response.
What you did not get was license to proceed and push this change. That's
because what is actually missing is the justification he asked for. As
Andrew pointed out the change is simple but the reason for implementing
it is not.
regards,
Andrew Dinn
---
Red Hat Distinguished Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill
Re: Possible subtle memory model error in ClassValue
On 11/08/2020 18:06, Hans Boehm wrote:
> I think the relevant statement is:
>
> "An address dependency between two reads generated by SVE vector load
> instructions does not contribute to the Dependency-ordered-before relation."
>
> This is only an issue if BOTH loads are SVE loads. In particular
> reference loads have to be vectorized for this to matter, which I expect
> is not the common situation. I have not explored this in great detail,
> but it should suffice to put fences between two dependent vector
> reference loads, and between a reference load and a dependent final
> field load.
Hmm, so this might perhaps be an issue with something like a deep copy
of an int[][], where loading of successive int[] references might be
vectorized using SVE instructions and processing of the contents of each
referenced int[] might also be similarly vectorized. In that case the
loading of the int contents would need to be ordered wrt the load of the
int[] references using a LoadLoad barrier?
regards,
Andrew Dinn
---
Red Hat Distinguished Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill
Re: Possible subtle memory model error in ClassValue
On 11/08/2020 11:47, Andrew Haley wrote:
> On 09/08/2020 18:55, Hans Boehm wrote:
>> There is no guarantee that the address dependency enforces ordering
>> if there is no final field involved. This may matter in the future,
>> since ARM's Scalable Vector Extension does not guarantee ordering
>> for address-dependent loads, if both loads are vector loads.
>
> Ouch. Thanks, I didn't know that.
You ought to look at the pdf Ningsheng linked in the RFR that was posted
with the SVE patch. The pdf is available here:
https://developer.arm.com/docs/ddi0584/latest
The relevant text is in section 4.4. Memory Ordering.
regards,
Andrew Dinn
---
Red Hat Distinguished Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill
Re: RFR: 8244853 - The static build of libextnet is missing the JNI_OnLoad_extnet function
Hi Alan,
On 12/05/2020 19:57, Alan Bateman wrote:
> Looks okay to me.
I am glad to hear that and also to see this fixed quickly (thanks Bob
for posting the patch). I'd like to see it backported to jdk11u (via
jdk14u, of course) if possible in order to allow stock jdk14u/11u to be
used to build GraalVM native images.
I was unaware that this was a spec requirement for static libs. Can you
point me at any discussion that explains why this is required and what
the function might be expected to do? or even tell me when it was added
to the spec? The macro appears to implement an empty function. Is there
any expectation/requirement that it could/should do something more than
that?
I am also wondering if this is the only such omission or if other libs
might require a corresponding JNI_OnLoad_extnet_xxx function.
regards,
Andrew Dinn
---
> On 12/05/2020 19:46, Bob Vandette wrote:
>> BUG:
>>
>> https://bugs.openjdk.java.net/browse/JDK-8244853
>>
>> Please review this simple fix for JDK 15 which adds the required
>> JNI_OnLoad_extnet function to the libextnet.a
>> static library when it is built.
>>
>> the JDK 15 make static-libs-image target currently builds this static
>> library but it is not spec compliant and
>> causes the GraalVM native-image utility to fail generating executables
>> due to the lack of the OnLoad symbol.
>>
>>
>> CHANGE:
>>
>> diff --git a/src/jdk.net/linux/native/libextnet/LinuxSocketOptions.c
>> b/src/jdk.net/linux/native/libextnet/LinuxSocketOptions.c
>> --- a/src/jdk.net/linux/native/libextnet/LinuxSocketOptions.c
>> +++ b/src/jdk.net/linux/native/libextnet/LinuxSocketOptions.c
>> @@ -34,6 +34,11 @@
>> #include "jdk_net_LinuxSocketOptions.h"
>> /*
>> + * Declare library specific JNI_Onload entry if static build
>> + */
>> +DEF_STATIC_JNI_OnLoad
>> +
>> +/*
>> * Class: jdk_net_LinuxSocketOptions
>> * Method: setQuickAck
>> * Signature: (II)V
>> diff --git a/src/jdk.net/macosx/native/libextnet/MacOSXSocketOptions.c
>> b/src/jdk.net/macosx/native/libextnet/MacOSXSocketOptions.c
>> --- a/src/jdk.net/macosx/native/libextnet/MacOSXSocketOptions.c
>> +++ b/src/jdk.net/macosx/native/libextnet/MacOSXSocketOptions.c
>> @@ -32,6 +32,11 @@
>> #include
>> #include "jni_util.h"
>> +/*
>> + * Declare library specific JNI_Onload entry if static build
>> + */
>> +DEF_STATIC_JNI_OnLoad
>> +
>> static jint socketOptionSupported(jint sockopt) {
>> jint one = 1;
>> jint rv, s;
>> diff --git
>> a/src/jdk.net/solaris/native/libextnet/SolarisSocketOptions.c
>> b/src/jdk.net/solaris/native/libextnet/SolarisSocketOptions.c
>> --- a/src/jdk.net/solaris/native/libextnet/SolarisSocketOptions.c
>> +++ b/src/jdk.net/solaris/native/libextnet/SolarisSocketOptions.c
>> @@ -32,6 +32,11 @@
>> static int initialized = 0;
>> /*
>> + * Declare library specific JNI_Onload entry if static build
>> + */
>> +DEF_STATIC_JNI_OnLoad
>> +
>> +/*
>> * Class: jdk_net_SolarisSocketOptions
>> * Method: init
>> * Signature: ()V
>
--
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill
Re: Improving ZipFile.getEntry performance using Bloom filters
On 14/04/2020 23:34, Ioi Lam wrote:
> I am a little worried adding extra overhead unconditionally into the JAR
> reader that people may not need/want.
A reasonable worry. We should always try to avoid fixes that benefit a
majority if they 'punish' a minority . . .
> Maybe we should take a step back and see why there are so many misses?
> Is it because you have a long classpath with many JARs on it, and you
> end up searching a lot of JAR files unnecessarily?
Well, there are a fair few as can be identified simply by Googling the pom
https://github.com/spring-projects/spring-petclinic/blob/master/pom.xml
and reading the dependencies section. n.b. that only shows top-level
dependencies but not recursive ones.
Unfortunately, this is going to be the reality for a many existing and
new apps. Most production Java apps are built from many library jars.
Java is the biggest 'divide and conquer' programming eco-system we have
ever seen in the history of computing. That's a direct corollary of it
being the biggest eco-system we have ever seen -- scale /necessitates/
divide and conquer.
> If this is the case, I think converting the app to modules will give you
> the speed up. A package can exist only in a single module so lookup is
> fast. You won't have misses unless you intentionally look for things
> that don't exist.
Well, yes but that also is just not going to fly for the majority of
Java developers for the reason given above. Most app developers are not
in a position to modularize their apps because the libraries they depend
on are not modularized, because the libraries /they/ depend on are not
modularized, because the libraries /they/ depend on are not modularized
... and so on. It's rarely going to be one group or organization with
one fixed goal that would need to schedule and implement such a change.
Now, you may lament that situation (or not) but it /is/ a brute fact and
is going to remain the status quo for a very long time. An eco-system of
the size of Java is like an ocean-liner. Which means the above advice is
going to whistle over the heads of many Java developers.
> Or, you can just package the app into one giant JAR file.
Again, that completely misses the reality of most developer's circumstances.
Now, I hope I don't come across like I am simply being negative here. I
have posted this reply because it's critically important that we, the
OpenJDK project devs, understand and keep in mind how most app
developers use (are able to use) Java. Suggestions that bypass the
realities and limitations of that usage say to me that we are at risk of
not meeting those users' needs.
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill
Re: RFR: (T) 8241144 Javadoc is not generated for new module jdk.nio.mapmode
On 24/03/2020 09:10, Magnus Ihse Bursie wrote:
> On 2020-03-24 09:59, Andrew Dinn wrote:
>> I'm assuming that also implies it is trivial (because, copyright update
>> a side, it really is a 1-liner :-).
>
> For code in the build system, we do not have the Hotspot rules of
> multiple reviewers, waiting period or trtiviality. A single OK review is
> enough to be allowed to push it.
Ah ok, thanks for the advice. I'll push as soon as hg.openjdk.java.net
comes back to life.
> (And for the record, you can add me as reviewer as well, if you wish :))
You are on the list :-)
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill
Re: RFR: (T) 8241144 Javadoc is not generated for new module jdk.nio.mapmode
On 23/03/2020 18:38, Erik Joelsson wrote:
> Looks good.
Thanks for the review, Erik.
I'm assuming that also implies it is trivial (because, copyright update
a side, it really is a 1-liner :-).
I will push to the dev tree and request a backport to jdk14u.
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill
> On 2020-03-23 10:56, Andrew Dinn wrote:
>> Could I please have a review for this trivial fix to the module make
>> file which ensures that javadoc is generated for new module
>> jdk.nio.mapmode created as part of the implementation of JEP 352. The
>> original patch added the module to the BOOT_MODULES list but was not to
>> the DOCS_MODULES list.
>>
>> JIRA: https://bugs.openjdk.java.net/browse/JDK-8241144
>> webrev: http://cr.openjdk.java.net/~adinn/8241144/webrev
>>
>> Testing:
>>
>> Built image and compiled + ran Hello World.
>>
>> Built make target docs-jdk-api-javadoc and checked module
>> jdk.nio.mapmode was included in output
>>
>> regards,
>>
>>
>> Andrew Dinn
>> ---
>> Senior Principal Software Engineer
>> Red Hat UK Ltd
>> Registered in England and Wales under Company Registration No. 03798903
>> Directors: Michael Cunningham, Michael ("Mike") O'Neill
>>
>
RFR: (T) 8241144 Javadoc is not generated for new module jdk.nio.mapmode
Could I please have a review for this trivial fix to the module make
file which ensures that javadoc is generated for new module
jdk.nio.mapmode created as part of the implementation of JEP 352. The
original patch added the module to the BOOT_MODULES list but was not to
the DOCS_MODULES list.
JIRA: https://bugs.openjdk.java.net/browse/JDK-8241144
webrev: http://cr.openjdk.java.net/~adinn/8241144/webrev
Testing:
Built image and compiled + ran Hello World.
Built make target docs-jdk-api-javadoc and checked module
jdk.nio.mapmode was included in output
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill
Re: RFR: 8239563 - Reduce public exports in dynamic libraries built from static JDK libraries
Hi Bob, On 25/02/2020 20:37, Bob Vandette wrote: > > Please review this RFE that alters the visibility of JNI entrypoints to > hidden when a shared library > is created using static JDK libraries. As David Holmes mentions in his ocmment on the JIRA it is possible to control re-export of the JNI entrypoints in the shared library using a linker map. Indeed, OpenJDK does this to limit visibility of entrypoints to libjvm. Is there a reason why this is not a viable alternative to changing the definition of the JNIEXPORT macros? regards, Andrew Dinn ---
Re: New candidate JEP: 371: Hidden Classes
On 26/01/2020 12:18, Michael Paus wrote:
> Am 26.01.20 um 13:00 schrieb core-libs-dev-requ...@openjdk.java.net:
>> This would help the reflective support for inline classes and
>> new VM/language feature such that adding support in MethodHandle
>> will work both both.
>
> In that case watch out for GraalVM. Native-image does not fully support
> MethodHandles.
>
> See:
> https://github.com/oracle/graal/blob/master/substratevm/LIMITATIONS.md#invokedynamic-bytecode-and-method-handles
I think you have put the cart before the horse here. Surely, it is the
Graal project that needs to watch out.
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill
Re: RFR JDK-8234049: Implementation of Memory Access API (Incubator)
Hi Maurizio,
It's nice to see this squeezing into jdk14.
On 09/12/2019 19:23, Maurizio Cimadamore wrote:
> Another iteration:
>
> http://cr.openjdk.java.net/~mcimadamore/panama/8234049_v4/
I eyeballed all the changes to the Buffer classes and saw no issues.
Also, apologies for the mixed brace/nobrace else-if cascade in
FileChannelImpl.java spotted by Roger which was my fault -- I should
really have introduced braces uniformly across the whole if/else-if/else
sequence.
Also, I reran pmem tests to verify that your buffer changes don't
disrupt the behaviour of sync mapped byte buffers and all is well.
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill
Re: RFR: 8232613: Move Object.registerNatives into HotSpot
On 22/10/2019 12:13, Claes Redestad wrote:
> No wait, I missed the use in jni.cpp where the bool return is actually
> used. I ignored it in systemDict since when I got an exception there
> (by misspelling a name or using the wrong signature) the exception
> bubbles up and crashes the VM.
>
> Perhaps this could be reworked to remove the bool cleanly, but let's go
> back to open.01 and move that cleanup to a follow-up.
Sure, no problem.
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill
Re: RFR: 8232613: Move Object.registerNatives into HotSpot
On 22/10/2019 12:05, Claes Redestad wrote:
> On 2019-10-22 12:44, Andrew Dinn wrote:
>> Why not leave it void?
>
> I guess:
>
> http://cr.openjdk.java.net/~redestad/8232613/open.02/
Ship it ;-)
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill
Re: RFR: 8232613: Move Object.registerNatives into HotSpot
On 22/10/2019 11:38, Claes Redestad wrote: > how about this: > > http://cr.openjdk.java.net/~redestad/8232613/open.01/ Sure that works. It's a bit more obvious now though that you are pointlessly returning a boolean from Method::register_native (this was also there in the previous version). Why not leave it void? regards, Andrew Dinn ---
Re: RFR: 8232613: Move Object.registerNatives into HotSpot
Hi Claes/David, On 22/10/2019 00:08, David Holmes wrote: > My only nit with this is that I don't think register_native and friends > belongs in the SystemDictionary class as it has nothing to do with the > SD. This code seems to be all about Methods so that seems like the place > to put this. Claes, the patch looks good. David, I'm not clear why you think this is about methods. The calls to register_native occur under SystemDictionary::resolve_well_known_classes which indicates to me that this code is very much about Classes/klasses -- specifically initializing class Object during early bootstrap. The fact that it is /methods/ of Object that are being fixed up doesn't really change that for me. So, I would have thought it would be clearer for the code that does this specific bit of 'well known class init' to be located with the other 'well known class init' code. regards, Andrew Dinn ---
Re: RFR: 8224974: Implement JEP 352
On 19/08/2019 15:36, Alan Bateman wrote:
> I think webrev.12 looks good; I don't have any other comments.
Thanks, Alan. I just pushed the patch for the JEP implementation.
(Hallelujah!). Thanks for all your help.
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Re: RFR: 8224974: Implement JEP 352
On 19/08/2019 19:38, Dmitry Chuyko wrote: > Just a minor style thing in MapSyncFail test: can "true" and "false" > (the mode) be "READ_WRITE_SYNC" and "READ_ONLY_SYNC" instead? Are you referring to the arguments passed on the command line? If so then the answer is clearly yes. However, I'm not sure why you consider this change important and/or an improvement? Do you feel that the code which processes the argument to compute the Mode setting is obscure? Or perhaps you are referring to something other than the command line arguments? I /do/ need to fix the typo in the test where is says "expected true of false as an argument" Obviously that should say "true *or* false". regards, Andrew Dinn ---
Re: RFR: 8224974: Implement JEP 352
Hi Alan,
Thanks for looking at the patch again. I think I have addressed all your
concerns (comments inline). Webrev and retest results at the end.
On 16/08/2019 11:39, Alan Bateman wrote:
> I think the main changes since I looked at it previously have been in
> the tests.
That's mostly it. I did also fix a problem that was breaking build of
x86_32. I also ensured that MAP_SYNC maps fail as expected on that arch.
> On non-Linux platforms, FileChannel.map should throw UOE when invoked
> with a mode map of READ_ONLY_SYNC or READ_WRITE_SYNC so I think we need
> a test for that.
>
> MapFail seems fragile. If you add @modules java.base/jdk.internal.misc
> to the test description then it could use Unsafe::isWritebackEnabled and
> I think would simplify the test and checks. It would mean it could run
> on all platforms. Also "MapFail" is probably too general a name for a
> test in MBB because its specific to the extended map modes, not a
> general test of map failing.
I renamed the test to MapSyncFail and modified it to run without
restriction to a specific os or cpu.
I also generalized it to run twice with a boolean arg which selects mode
READ_ONLY_SYNC on the first run and READ_WRITE_SYNC on the second one.
The logic of the test is now to expect
1) IOException if Unsafe.isWriteBackEnabled -> true
2) UnsupportedOperationException if Unsafe.isWriteBackEnabled -> false
If the wrong exception or neither exception is thrown the test fails.
Case 1 currently only applies for x86_64.
Case 2 applies for all other architectures.
> In passing, MappedByteBuffer load/isLoaded check the fd value before
> isSync, can force() do the same? Also the @return on the private isSync
> method is very wordy and I don't think needs to duplicate the method
> description.
Sure, I have modified force() to do that check first.
Of course, that means that force(int, int) is going to repeat the same
test -- it has to because it may be called direct without going via force().
However, that's not really a problem since the compiler should elide the
repeated check.
I also shortened the text following the @return annotation as requested.
Updated webrev:
http://cr.openjdk.java.net/~adinn/8224974/webrev.12
Testing:
Test PmemTest:
passes as expected on x86_64 (only arch for which DAX file system is
available)
fails to pass as expected on aarch64 and x86_32 (however, this case is
covered by the next test)
Test MapSyncFail:
passes with expected exceptions on Linux for x86_64 (IOException),
aarch64 and x86_32 (UnsupportedOperationException).
not tested on other arch/OS combinations (I have no access to the
necessary kit).
Red Hat MW tests:
All still passing successfully
submit test:
still in progress
Is it ok to push if the submit test comes back clean?
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Re: RFR: 8224974: Implement JEP 352
On 07/08/2019 14:32, Dmitry Chuyko wrote:
> The test fails on some machines but does not fail on others, all 4.x
> kernels. The possible problem may be when build host and run host are
> different machines. This seems to be related to map0() implementation in
> FileChannelImpl.c in case MAP_SYNC and MAP_SHARED_VALIDATE are not
> defined (or defined).
Ah yes, I see what is goign on now. It makes sense that you are seeing
IOException: "Invalid argument" when passing MAP_SYNC to mmap on a Linux
kernel that does not support those flags. Whereas on a Linux kernel
which does support those flags you would expect to see the result i got
IOException: "Operation not supported". That's because mmap with
MAP_SYNC is not an appropriate operation to request on a non-DAX file.
n.b. these messages come out of the relevant errno value.
The AArch64 code produces an UnsupportedOperationException because the
map with MAP_SYNC is being rejected in the Java code. That happens
because I am running on an ARMV8.1 processor which does not support
cache writeback to memory (ARMv8.2 chips are not yet generally available).
The same applies for i386. It produces UnsupportedOperationException
because the map with MAP_SYNC is being rejected in the Java code.
The test needs tweaking to remedy this bad result and explain better the
expected results. I don't think there is actually any great merit
checking for a specific error message in the IOException so I will
remove that check. The test should always expect IOException from x86,
always expect UnsupportedOperationException from i386 and allow for
either from AArch64. I have folded those fixes into the next version.
> I also recommend to print original ioe stacktrace in the test. Adding
> such gives us useful information like this:
I have already done this too :-)
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Re: RFR: 8224974: Implement JEP 352
On 07/08/2019 13:04, Dmitry Chuyko wrote:
> On 8/7/19 2:02 PM, Dmitry Chuyko wrote:
>> Andrew,
>>
>> New code is buildable and new MapFail test passes on different
>> platforms except it fails on linux i386:
>
> Ah, that even was x86_64 (sorry, mixed up results from automated
> system). I'll try to reproduce it by hand to see if there are any
> additional details.
Whew! Thank goodness it is x86_64. You had me very worried when you said
this was i386 :-)
So, this is indeed a problem with the test. The internal detail message
for the IOException will vary according to the Linux kernel release. If
MAP_SYNC is not supported by the Linux kernel it will embed the message
you saw. If it is supported the message will be the one the test checks for.
I think the best thing is for the test not to bother checking for a
specific message. The important thing is that we should get either
IOException or UnsupportedOperationException. Checking for the message
is not really necessary. I will remove this check from the test.
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Re: RFR: 8224974: Implement JEP 352
Hello Dmitry,
On 06/08/2019 15:25, Dmitry Chuyko wrote:
> One quick question about synchronization in unmappers. One of
> preliminary steps for Loom was to replace monitor usage by j.u.c locks
> for I/O to let fibers release carrier threads. For instance see
> JDK-8222774. Does it make sense to do the same in your new unmappers code?
> . . .
> [1] https://bugs.openjdk.java.net/browse/JDK-8222774
The unmapper code is not strictly 'new' as regards its reliance on
synchronization. It merely follows and repeats the pattern employed in
the prior code that it has generalized (by splitting the original
Unmapper into two distinct flavours of subclass).
If this poses a problem for Loom then it is a separate issue form the
one this JEP addresses. I think you should raise a new issue for that
change (just as you would have had to do before this change). I am sure
Alan Bateman will be happy to consider your proposal. Indeed, I would be
happy to implement it given his approval -- or leave it to you to do so
if you prefer.
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Re: RFR: 8224974: Implement JEP 352
Hello Dmitry,
On 06/08/2019 15:50, Dmitry Chuyko wrote:
> Thanks for previous detailed explanations, and great, there are now
> checks for exceptions in webrev.11. Let me add just a few comments about
> that.
Sure, no problem. Thank you for looking at the patch.
> New
> Unsafe.writebackMemory()/checkWritebackMemory()/checkWritebackEnabled()
> @throws RuntimeException "if memory writeback is not supported...".
> There is no test that check this behavior of Unsafe class while it seems
> to be relatively simple as inner check actually operates with a constant
> value.
No this behaviour is not currently tested. However, the only client at
present will never exercise that path so it is not critical to test it
now. I'd be happy to address testing of this behaviour as part of a
follow-up JIRA issue if you would be so good as to raise it. I say that
because I would very much like to get this functionality into a release
to simplify more extensive testing by Red Hat's middleware teams.
> New MapFail test succeeds if proper IOException or any
> UnsupportedOperationException was caught but it aren't those situations
> actually 2 different ones that require distinct checks? If you say that
> is the situation when results depend on Linux version, it makes sense at
> least to put a comment in the test because it's really not trivial.
The documentation of the API under test makes it clear that both errors
can occur and under what circumstances. However, a note in the test will
certainly do no harm. I will insert one before checking in the patch.
> Can PmemTest check IOException with message "map with mode MAP_SYNC
> unsupported" as a part of expected behavior, not just showing a test
> failure?
I don't see any need for this now that MapFail has been provided. Wit
that alterative in place for checking map failures on non-DAX file
syetems PmemTest is now primarily intended to test behaviour with a DAX
file system which it expects to have been set up in advance as described
in the main comment. So, the scenario you describe is not really an
intended usage and I woudl argue that a failure is the right way to
signal that.
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Re: RFR: 8224974: Implement JEP 352
On 06/08/2019 13:44, Aleksey Shipilev wrote:
> Ah, that is exactly what I wanted. Good then, scratch the rest of my comments.
> . . .
> I thought that translating two separate (and statically bound) Unsafe calls,
> hooking them up to
> separate Unsafe leaf entries, and then suddenly going into a single
> StubRoutine call with dynamic
> argument that dispatches at runtime is a bit awkward. I would have expected
> it to end up with two
> separate StubRoutines as well. Again, I have no strong opinion about this.
Ok, thanks for clarifying. Inertia dictates I leave the stubs as is :-)
> Yes, this looks cleaner. The declarations can be a bit less crowded:
>
> static address data_cache_writeback() { return _data_cache_writeback; }
> static address data_cache_writeback_sync() { return
> _data_cache_writeback_sync; }
>
> typedef void (*DataCacheWritebackStub)(void *);
> static DataCacheWritebackStub DataCacheWriteback_stub() { return ...
>
> typedef void (*DataCacheWritebackSyncStub)(bool);
> static DataCacheWritebackSyncStub DataCacheWritebackSync_stub() { return ...
> . . .
>> http://cr.openjdk.java.net/~adinn/8224974/webrev.11
>
> Looks good.
Ok, I'll fold this and the other format errors you identified into the
next patch.
If I could please get a nod from Alan Bateman (and assuming I don't
receive further comments from other reviewers) I'll push that next patch.
Any more for any more ... ?
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Re: RFR: 8224974: Implement JEP 352
Hi Aleksey/Boris,
This is a response to both your last review posts. New webrev link is at
the end.
On 01/08/2019 12:16, Aleksey Shipilev wrote:
> On 7/31/19 12:55 PM, Andrew Dinn wrote:
. . .
> I am more concerned that the writeback call enters the pre sync stub
> unnecessarily.
The stub? I hope you mean when executing the native call as opposed to
the JITted intrinsic? The stub is only called on a cold path when a
native call proper happens. By contrast, the intrinsic translation for
CacheWBPreSync on AArch64 and x86_64 does not insert any instructions
into the generated code (and most especially not a call to the stub).
Here it is:
instruct cacheWBPreSync()
%{
predicate(VM_Version::supports_data_cache_line_flush());
match(CacheWBPreSync);
ins_cost(100);
format %{"cache wb presync" %}
ins_encode %{
__ cache_wbsync(true);
%}
ins_pipe(pipe_slow); // XXX
%}
void MacroAssembler::cache_wbsync(bool is_pre)
{
assert(VM_Version::supports_clflush(), "clflush should be available");
bool optimized = VM_Version::supports_clflushopt();
bool no_evict = VM_Version::supports_clwb();
// pick the correct implementation
if (!is_pre && (optimized || no_evict)) {
// need an sfence for post flush when using clflushopt or clwb
// otherwise no no need for any synchroniaztion
sfence();
}
}
> I had the idea to do this more efficiently, and simplify code at the same
> time: how about emitting
> CacheWBPreSync nodes, but emitting nothing for them in .ad matchers? That
> would leave generic code
> generic, and architectures would then be able to avoid the stub altogether
> for pre sync code. This
> would simplify current stub generators too, I think: you don't need to pass
> arguments to them.
I believe the intrinsic behaviour you are asking for is effectively what
is implemented (as shown above). The .ad match rules for the PreSync and
PostSync nodes both call MacroAssembler::cache_wbsync. For pre-sync it
emits nothing. For post-sync it emits sfence when the writeback is
implemented using clwb or clflushopt and nothing if writeback relies on
clflush.
> This leaves calling via Unsafe. I believe pulling up the isPre choice to the
> stub generation time
> would be beneficial. That is, generate *two* stubs:
> StubRoutines::data_cache_writeback_pre_sync()
> and StubRoutines::data_cache_writeback_post_sync(). If arch does not need the
> pre_sync, generate nop
> version of pre_sync().
I don't really see any point to doing this. We can generate two stubs,
one executing a nop and one executing a nop/sfence according to need. Or
we can have one stub with a branch on the sync type and branch targets
that execute either a nop or a nop/sfence as needed. The difference in
performance of the stub is minor and irrelevant. The difference in
generation time and memory use is minor and irrelevant. What are you
trying to gain here?
> This is not a strong requirement from my side. I do believe it would make
> code a bit more
> straight-forward.
Am I missing something here? Or did you simply miss that the intrinsic
translation inserts no code for the presync?
>>> === src/hotspot/cpu/x86/assembler_x86.cpp
>>>
>>> It feels like these comments are redundant, especially L8630 and L8646
>>> which mention magic values
>>> "6" and "7", not present in the code:
>
> ...
>
>> 8624 // 0x66 is instruction prefix
>>
>> 8627 // 0x0f 0xAE is opcode family
>>
>> 8630 // rdi == 7 is extended opcode byte
>> . . .
>>
>> Given that the code is simply stuffing numbers (whether supplied as
>> literals or as symbolic constants) into a byte stream I think these
>> comments are a help when it comes to cross-checking each specific
>> assembly against the corresponding numbers declared in the Intel
>> manuals. So, I don't really want to remove them. Would you prefer me to
>> reverse the wording as above?
>
> I was merely commenting on the style: the rest of the file does not have
> comments like that. The
> positions of prefixes, opcode families, etc is kinda implied by the code
> shape.
Yes, I too noticed that the rest of the file does not have any such
comments :-]
Given the highly variable shape of x86 machine code, I don't see any
reason not to start remedying that omission, even if the remedy is only
piecemeal. Commenting may not be a great help to maintainers who know
the code and ISA really well but they are not the only audience. Even in
that specific case the comments provide a sanity check.
>>> === src/hotspot/cpu/x86/macroAssembler_x86.cpp
>> // prefer clwb (potentially parallel writeback without evict)
>> // otherwise prefer clflushopt (potentially parallel
Re: RFR: 8224974: Implement JEP 352
Hi Boris,
On 31/07/2019 13:01, Boris Ulasevich wrote:
> I did a quick check of the change across our platforms. Arm32 and x86_64
> built successfully. But I see it fails due to minor issues on aarch64
> and x86_32 with webrev.09.
> Can you please have a look at this?
>
>> src/hotspot/cpu/aarch64/aarch64.ad:2202:1: error: expected ‘;’ before
> ‘}’ token
>> src/hotspot/cpu/x86/macroAssembler_x86.cpp:9925: undefined reference
> to `Assembler::clflush(Address)'
The AArch64 error was simply a missing semi-colon. With that corrected
AArch64 now builds and runs as expected (i.e. it fails the PMem support
test with an UnsupportedOperationException).
The second error is happening because the calling method
MacroAssembler::cache_wb has not been guarded with #ifdef _LP64 (the
same applies for MacroAssembler::cache_wbsync). Note that cache line
writeback via Unsafe.writeBackMemory is only expected to work on Intel
x86_64 so these two methods only get called from x86_64-specific code
(x86_64.ad and stuGenerator_x86_64.cpp).
So, the solution to this specific problem is to add #ifdef _LP64 around
the declaration and implementation of these two methods. At the same
time it would be helpful to remove the redundant #ifdef _LP64/#endif
that I for some strange reason inserted around the definitions, but not
the declarations, of clflushopt and clwb (that didn't help when I was
trying to work out what was going wrong).
However, a related problem also needs fixing. The Java code for method
Unsafe.writebackMemory only proceeds when the data cache line writeback
unit size (value of field UnsafeConstants.DATA_CACHE_LINE_FLUSH_SIZE) is
non-zero. Otherwise it throws an exception. On x86_32 that field /must/
be zero. The native methods which Unsafe calls out to and the intrinsics
which replace the native calls are not implemented on x86_32.
The field from which the value of the Java constant is derived is
currently initialised using CPU-specific information in
vm_version_x86.cpp as follows
if (os::supports_map_sync()) {
// publish data cache line flush size to generic field, otherwise
// let if default to zero thereby disabling writeback
_data_cache_line_flush_size =
_cpuid_info.std_cpuid1_ebx.bits.clflush_size * 8;
}
i.e. writeback is enabled on x86 when the operating is known to be
capable of supporting MAP_SYNC. os_linux.cpp returns true for that call,
irrespective of whether this is 32 or 64 bit linux. The rationale is
that any Linux is capable of supporting map_sync (by contrast Windows,
Solaris, AIX etc currently return false). So, the above assignment also
needs to be guarded by #ifdef _LP64 in order to ensure that writeback is
never attempted on x86_32.
Thank you for spotting these errors. I will add the relevant fixes to
the next patch and add you as a reviewer.
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Re: RFR: 8224974: Implement JEP 352
Hello Dmitry,
On 01/08/2019 05:09, Dmitry Chuyko wrote:
> Just a small comment about the tests. As you can see, some of tests in
> jdk/java/nio/channels/FileChannel check various modes, arguments, and
> expected exceptions. E.g. see MapTest, Mode and Args.
>
> I noticed that in your changes for the new map mode there are new
> exceptions thrown in different situations. For example, when the
> required mode is not supported, or it does not fit. In particular, this
> should happen correctly on systems that do not support nvram. Have you
> considered the possibility of extending or adding tests for this behavior?
I agree that these failure cases need better test coverage and
automation. However, that is not to say they have not been tested. All
current success and failure cases can be exercised by the current mmap
test (PmemTest) if run in the correct circumstance. Unfortunately,
automatic testing is not straightforward.
1) On x86_64 where MAP_SYNC is supported test PMemTest includes
instructions to exercise a successful map from a real or emulated DAX
file system. It can also be used (and has been used) to exercise the
IOException failure path in the case where the file it tries to open
belongs to a non-DAX file system (see line 99).
Note that testing for success or failure cannot be done automatically
using the test as it currently stands. Testing for a successful map
requires mounting a real or emulated DAX file system at a known mount
point and creating a writable dir to hold the mapped file. Testing for
the IOException failure requires either setting up an equivalent non-DAX
file system mount or editing the test to open the file in a non-DAX file
system dir like, say, /tmp.
A new, separate test for the IOException failure could be automated on
x86_64 by trying to map a file located in /tmp (on the assumption that
/tmp is not mounted as a DAX file system). Of course, at present this
will only give the IOException result when MAP_SYNC is supported. Given
a suitably old Linux/x86_64, UnsupportedOperationException could also be
thrown.
2) On AArch64 where MAP_SYNC is not currently supported PmemTest clearly
cannot be used to exercise the success path. However, it can be used
(and has been used) to exercise the UnsupportedOperationException
failure path.
A check for the UnsupportedOperationException failure could be automated
on AArch64 by a new test as described above. Of course, once MAP_SYNC
support arrives in a new Linux/AArch64 it would also become for
IOException to be thrown.
So, to sum up, it would be possible to add a new, automatic test that
checks for one or other failure occurring. I am happy to add such a test
to the next webrev.
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Re: RFR: 8224974: Implement JEP 352
On 31/07/2019 16:46, Aleksey Shipilev wrote:
> On 7/31/19 4:46 PM, Andrew Dinn wrote:
>> Well, the failure happened during the build process so I didn't
>> (couldn't) debug it. I disabled the assert in supports_cpuflush() in
>> order to allow the build to complete and then ran up the resulting JVM
>> inside gdb to check what was going on. The problem is that with my patch
>> supports_cpuflush() is called from Assembler::clflush() and the latter
>> is called from icache_x86.cpp very early during bootstrap (I think this
>> is neded to flush the flush routine used to flush the code cache).
>> Anyway, the call happens so early that it precedes the call to
>> VM_Version::get_processor_features which sets up the _features mask
>> tested by the assert.
>
> I believe you can untie this bootstrapping circularity by relaxing the assert
> with
> Universe::is_fully_unitialized(). It still gives us window to fail with
> SIGILL if the stub is called
> early during bootstrap, but that would be something to fix *if* we ever find
> ourselves there.
Yes indeed, that seems like by far the cleanest solution. The only other
alternative is to move the call to VM_Version::initalize earlier in the
Universe bootstrap sequence and I doubt anyone wants to risk that.
So, now I have
#ifdef _LP64
static bool supports_clflush() {
// clflush should always be available on x86_64
// if not we are in real trouble because we rely on it
// to flush the code cache.
// Unfortunately, Assembler::clflush is currently called as part
// of generation of the code cache flush routine. This happens
// under Universe::init before the processor features are set
// up. Assembler::flush calls this routine to check that clflush
// is allowed. So, we give the caller a free pass if Universe init
// is still in progress.
assert ((!Universe::is_fully_initialized() || (_features &
CPU_FLUSH) != 0), "clflush should be available");
return true;
}
. . .
which seems to work ok.
I'll post a new webrev when I have handled Brian's comments and also any
other feedback you may still have on my last reply.
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Re: RFR: 8224974: Implement JEP 352
On 31/07/2019 12:40, Aleksey Shipilev wrote:
> On 7/31/19 1:29 PM, Andrew Dinn wrote:
>> I have an update regarding the change to the computation of the
>> CPU_FLUSH flag. After posting the new webrev I built a debug version
>> with the change that tests the clflush bit on x86_64. It crashed when
>> the assert is reached in VM_Version::supports_cpuflush:
>>
>> # A fatal error has been detected by the Java Runtime Environment:
>> #
>> # Internal Error (src/hotspot/cpu/x86/vm_version_x86.hpp:978),
>> pid=29597, tid=29602
>> # assert((_features & ((uint64_t)(0x200ULL))) != 0) failed:
>> clflush should be available
>>
>> So, it seems the clflush bit is not set on my x86_64 box even though I
>> am able to execute clflush quite happily.
>>
>> "Toto, it looks like we are no longer in Antarctica."
>>
>> So, I will revert this change (in the next webrev).
>
> But wait, that might mean the clflush is indeed not available, by either
> hardware or software
> switch? I believe some security mitigations disable clflush. Linux kernel,
> for example, has
> "noclflush" option to do this. Ignoring the cpu bit and emitting clflush
> anyway might circumvent
> that mitigation then?
Ok, so investigating further I have found out what is wrong here.
Significanlty, I get this result when I look at /proc/cpuinfo.
[root@localhost ~]# cat /proc/cpuinfo | grep flush | head -1
flags : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov
pat pse36 *clflush* dts acpi mmx fxsr sse sse2 ...
So, the hardware does flag support for clflush. In which case why does
the assert in supports_clflush fail?
Well, the failure happened during the build process so I didn't
(couldn't) debug it. I disabled the assert in supports_cpuflush() in
order to allow the build to complete and then ran up the resulting JVM
inside gdb to check what was going on. The problem is that with my patch
supports_cpuflush() is called from Assembler::clflush() and the latter
is called from icache_x86.cpp very early during bootstrap (I think this
is neded to flush the flush routine used to flush the code cache).
Anyway, the call happens so early that it precedes the call to
VM_Version::get_processor_features which sets up the _features mask
tested by the assert.
So, I think the only option is to remove the assert from the x86_64
version of supports_cpuflush from Assemmbler::cpu_flush and instead add
an assert in get_processor_features that CPU_FLUSH is set on x86_64.
However, by that stage on x86_64 we will already have called clflush
and, in the case where the assert might actually fail, we may not reach
there because we have tripped up with an illegal instruction error.
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Re: RFR: 8224974: Implement JEP 352
On 31/07/2019 13:01, Boris Ulasevich wrote:
> I did a quick check of the change across our platforms. Arm32 and x86_64
> built successfully. But I see it fails due to minor issues on aarch64
> and x86_32 with webrev.09.
> Can you please have a look at this?
>
>> src/hotspot/cpu/aarch64/aarch64.ad:2202:1: error: expected ‘;’ before
> ‘}’ token
>> src/hotspot/cpu/x86/macroAssembler_x86.cpp:9925: undefined reference
> to `Assembler::clflush(Address)'
Sure, will do.
I'm surprised by the x86_32 result as I /thought/ I had pushed the very
same change set to the submit repo and it came back with no errors:
> Job: mach5-one-adinn-JDK-8224974-8-20190730-1325-4068436
> BuildId: 2019-07-30-1324012.adinn.source
> No failed tests
. . .
Is x86_32 tested as part of a submit run?
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Re: RFR: 8224974: Implement JEP 352
Hi Aleksey, I have an update regarding the change to the computation of the CPU_FLUSH flag. After posting the new webrev I built a debug version with the change that tests the clflush bit on x86_64. It crashed when the assert is reached in VM_Version::supports_cpuflush: # A fatal error has been detected by the Java Runtime Environment: # # Internal Error (src/hotspot/cpu/x86/vm_version_x86.hpp:978), pid=29597, tid=29602 # assert((_features & ((uint64_t)(0x200ULL))) != 0) failed: clflush should be available So, it seems the clflush bit is not set on my x86_64 box even though I am able to execute clflush quite happily. "Toto, it looks like we are no longer in Antarctica." So, I will revert this change (in the next webrev). regards, Andrew Dinn --- On 31/07/2019 11:55, Andrew Dinn wrote: > Hi Aleksey > > On 30/07/2019 17:00, Aleksey Shipilev wrote: >> On 7/30/19 5:04 PM, Andrew Dinn wrote: >>> JEP 352 has now been targeted for inclusion in JDK14. The latest webrev >>> for the implementation JIRA has been rebased to apply to the current >>> tree. Is it now ok to push this change set? >>> >>> JIRA: https://bugs.openjdk.java.net/browse/JDK-8224974 >>> webrev: http://cr.openjdk.java.net/~adinn/8224974/webrev.09 >> Is it okay to have a late code review? Here it goes: > > Sure, welcome to the party even if most of the booze has been drunk :-) > > Thank you for a very thorough review. Comments (mostly in agreement) are > inline below. They include a few refusals and a walk past some of the > jumps pending confirmation they truly form part of the obstacle course. > > A new webrev against the latest jdk/jdk dev tree including all > uncontested changes is here: > > http://cr.openjdk.java.net/~adinn/8224974/webrev.10/ > >> === General: >> >> So if pre wbsync is no-op, why do we need to handle it everywhere? We seem >> to be falling through all >> the way to the stub to do nothing there, maybe we should instead cut much >> earlier, e.g. when >> inlining Unsafe.writeBackPresync0? Would it be better to not emit >> CacheWBPreSync at all? > > Ah, I see you brought your own bottle ;-) > > The pre sync is definitely not needed at present. However, I put it > there because I didn't know for sure if some future port of this > capability (e.g. to ppc) might need to sync prior writes before writing > back cache lines. [Indeed, the old Intel documentation stated that > pre-sync was needed on x86 for clflush to be safe but it is definitely > not needed.] > > Anyway, folding out the pre-sync as you suggest would mean taking what > is still potentially an architecture-specific decision in generic code. > That may well prove to be a redundant precaution but it also costs > little -- every affected /compile/ will last a tad longer and be a tad > fatter in its memory footprint. If you think change is important I will > happily oblige. > >> === General: >> >> IIRC, notproduct and PRODUCT defines are legacy, and should be avoided? >> develop and ASSERT must be >> the substitutes for this. See some discussion here: >> https://bugs.openjdk.java.net/browse/JDK-8183287 > > Ok done. > > I changed #ifndef PRODUCT to #ifdef ASSERT in library_call.cpp and > unsafe.cpp. I also redefined global flag TraceMemoryWriteback using > develop rather than notproduct. > > I also removed the AArch64 /product/ compiler option UsePOCForPOP. This > allowed the JIT to use a memory writeback instruction with weakened > semantics if the full writeback is not available in a product build. > product was chosen because this was needed to establish a benchmark for > writeback on existing kit when using a pseudo-NVRAM memory device based > on volatile memory. The point was to be able to do an apples to apples > comparison with writeback to a disk device. Obviously, it's not much > use doing that test with a a non-product build. This is only important > during development as a one-off test once the OS support for > pseudo-NVRAM devices arrives in Linux AArch64 (which should happen well > before new kit with the POP writeback arrives). I can still perform it > using a custom build so I dropped this from the patch. > >> === src/hotspot/cpu/aarch64/aarch64.ad >> src/hotspot/cpu/x86/x86.ad >> >> This should probably be just "!VM_Version...". Braces around the statement >> would not hurt either. >> >> 2196 if (VM_Version::supports_data_cache_line_flush() == false) >> 2197 ret_value = false; > > Done. > >> === src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp >> src/hotspot/cpu/x86/macroAssembler_x86.cpp >
Re: RFR: 8224974: Implement JEP 352
Hi Aleksey
On 30/07/2019 17:00, Aleksey Shipilev wrote:
> On 7/30/19 5:04 PM, Andrew Dinn wrote:
>> JEP 352 has now been targeted for inclusion in JDK14. The latest webrev
>> for the implementation JIRA has been rebased to apply to the current
>> tree. Is it now ok to push this change set?
>>
>> JIRA: https://bugs.openjdk.java.net/browse/JDK-8224974
>> webrev: http://cr.openjdk.java.net/~adinn/8224974/webrev.09
> Is it okay to have a late code review? Here it goes:
Sure, welcome to the party even if most of the booze has been drunk :-)
Thank you for a very thorough review. Comments (mostly in agreement) are
inline below. They include a few refusals and a walk past some of the
jumps pending confirmation they truly form part of the obstacle course.
A new webrev against the latest jdk/jdk dev tree including all
uncontested changes is here:
http://cr.openjdk.java.net/~adinn/8224974/webrev.10/
> === General:
>
> So if pre wbsync is no-op, why do we need to handle it everywhere? We seem to
> be falling through all
> the way to the stub to do nothing there, maybe we should instead cut much
> earlier, e.g. when
> inlining Unsafe.writeBackPresync0? Would it be better to not emit
> CacheWBPreSync at all?
Ah, I see you brought your own bottle ;-)
The pre sync is definitely not needed at present. However, I put it
there because I didn't know for sure if some future port of this
capability (e.g. to ppc) might need to sync prior writes before writing
back cache lines. [Indeed, the old Intel documentation stated that
pre-sync was needed on x86 for clflush to be safe but it is definitely
not needed.]
Anyway, folding out the pre-sync as you suggest would mean taking what
is still potentially an architecture-specific decision in generic code.
That may well prove to be a redundant precaution but it also costs
little -- every affected /compile/ will last a tad longer and be a tad
fatter in its memory footprint. If you think change is important I will
happily oblige.
> === General:
>
> IIRC, notproduct and PRODUCT defines are legacy, and should be avoided?
> develop and ASSERT must be
> the substitutes for this. See some discussion here:
> https://bugs.openjdk.java.net/browse/JDK-8183287
Ok done.
I changed #ifndef PRODUCT to #ifdef ASSERT in library_call.cpp and
unsafe.cpp. I also redefined global flag TraceMemoryWriteback using
develop rather than notproduct.
I also removed the AArch64 /product/ compiler option UsePOCForPOP. This
allowed the JIT to use a memory writeback instruction with weakened
semantics if the full writeback is not available in a product build.
product was chosen because this was needed to establish a benchmark for
writeback on existing kit when using a pseudo-NVRAM memory device based
on volatile memory. The point was to be able to do an apples to apples
comparison with writeback to a disk device. Obviously, it's not much
use doing that test with a a non-product build. This is only important
during development as a one-off test once the OS support for
pseudo-NVRAM devices arrives in Linux AArch64 (which should happen well
before new kit with the POP writeback arrives). I can still perform it
using a custom build so I dropped this from the patch.
> === src/hotspot/cpu/aarch64/aarch64.ad
> src/hotspot/cpu/x86/x86.ad
>
> This should probably be just "!VM_Version...". Braces around the statement
> would not hurt either.
>
> 2196 if (VM_Version::supports_data_cache_line_flush() == false)
> 2197 ret_value = false;
Done.
> === src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp
> src/hotspot/cpu/x86/macroAssembler_x86.cpp
>
> Braces style mismatch, should be on the same line, as in the rest of the file:
>
> 5837 void MacroAssembler::cache_wb(Address line)
> 5838 {
>
> 5856 void MacroAssembler::cache_wbsync(bool is_pre)
> 5857 {
Done.
> === src/hotspot/cpu/x86/assembler_x86.cpp
>
> It feels like these comments are redundant, especially L8630 and L8646 which
> mention magic values
> "6" and "7", not present in the code:
>
> 8624 // instruction prefix is 0x66
>
> 8627 // opcode family is 0x0f 0xAE
>
> 8630 // extended opcode byte is 7 == rdi
>
> 8640 // instruction prefix is 0x66
>
> 8643 // opcode family is 0x0f 0xAE
>
> 8646 // extended opcode byte is 6 == rsi
Well, I think the question of redundancy depends on how you look at it.
The comments are not there to explain that these values are the ones
being passed to the emit functions -- that is pretty obvious. They are
there to explain what those values mean i.e. to explain what elements of
the fully assembled instruction the emit calls assembling piecemeal.
Perhaps that reading woudl be clearer if each such equation of terms was
done in reverse order:
8624 /
Re: RFR: 8224974: Implement JEP 352
JEP 352 has now been targeted for inclusion in JDK14. The latest webrev
for the implementation JIRA has been rebased to apply to the current
tree. Is it now ok to push this change set?
JIRA: https://bugs.openjdk.java.net/browse/JDK-8224974
webrev: http://cr.openjdk.java.net/~adinn/8224974/webrev.09
n.b. by way of sanity test I pushed this to the submit repo and it came
back with no failed tests.
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Re: RFR: 8224974: Implement JEP 352
Hi Alan, On 02/07/2019 16:59, Alan Bateman wrote: > On 18/06/2019 12:43, Andrew Dinn wrote: > One nit is that the update to the Goals section says "rework" in two > places. I think it might more accurate to say "upgrade" or "update" as > it doesn't significantly re-working the existing implementation. Thanks for checking this rewording. I have updated the JEP text modulo replacing reworked with upgraded. regards, Andrew Dinn ---
Re: RFR: 8224974: Implement JEP 352
Hi Alan,
I have uploaded a webrev which I believe answers all outstanding review
comments:
JIRA: https://bugs.openjdk.java.net/browse/JDK-8224974
webrev: http://cr.openjdk.java.net/~adinn/8224974/webrev.08/
This includes the changes Joe Darcy highlighted in his code review of
the implementation CSR (module header comment plus @since 14 changes in
Unsafe.java). It also allows for the tweaks to the force API
documentation (JDK-8226203).
So, I think the only work still outstanding is to agree changes to the
JEP wording (as proposed in my previous post -- see below) plus
endorsement and targeting of the JEP.
regards,
Andrew Dinn
---
On 18/06/2019 12:43, Andrew Dinn wrote:
> Hi Alan,
>
> Thanks for reviewing the JEP one more time.
>
> On 16/06/2019 09:47, Alan Bateman wrote:
>> I re-read the JEP, trying to put myself in the position of someone
>> reading it for the first time in 2020.
>>
>> Summary section:
>>
>> What would you think about replacing this with a sentence that makes it
>> clear that the JEP adds new JDK-specific file mapping modes to allow the
>> FileChannel API create MappedByteBuffers over non-volatile memory?
>
> I would be happy with that way of describing the behaviour change except
> that what you suggest doesn't mention actually making it work -- which
> /is/ part of the JEP. How about:
>
> "Add new JDK-specific file mapping modes, allowing the FileChannel API
> to be used to create MappedByteBuffers over non-volatile memory, and
> extend the underlying implementation to support this new use case"
>
>> Goals section:
>>
>> I think the first paragraph could be re-worded to make it clear that the
>> goal is to use the existing MappedByteBuffer API to access NVM.
>
> Yes indeed:
>
> "This JEP proposes that class MappedByteBuffer be reworked to support
> access to non-volatile memory (NVM). The only API change required is a
> new enumeration employed by FileChannel clients to request mapping of a
> file located on an NVM-backed file system rather than a conventional,
> file storage system. Recent changes to the MappedByteBufer API mean that
> it supports all the behaviours needed to allow direct memory updates and
> provide the durability guarantees needed for higher level, Java client
> libraries to implement persistent data types (e.g. block file systems,
> journaled logs, persistent objects, etc.). The implementations of
> FileChannel and MappedByteBuffer need revising to be aware of this new
> backing type for the mapped file."
>
>> Paragraphs 2-5 split this into two sub-goals. The first suggests that it
>> extends the MBB API but this is no longer the case. The second also
>> hints that it adds a new API. I think these two need to be re-worded.
>
> Agreed. How about this:
>
> "The primary goal of this JEP is to ensure that clients can access and
> update NVM from a Java program efficiently and coherently. A key element
> of this goal is to ensure that individual writes (or small groups of
> contiguous writes) to a buffer region can be committed with minimal
> overhead i.e. to ensure that any changes which might still be in cache
> are written back to memory."
>
> n.b. I snuck in the word coherence because I thought it clarified the
> notion of committing to memory.
>
>> Goal 3 and 4 are okay, although I think the 4th could be summarized as
>> allowing mapped buffers on NVM to be tracked by the existing monitoring
>> and management APIs.
>
> Agreed. So, renumbering accordingly, how about this:
>
> "A second, subordinate goal is to implement this commit behaviour using
> a restricted, JDK-internal API defined in class Unsafe, allowing it to
> be re-used by classes other than MappedByteBuffer that may need to
> commit NVM.
>
> A final, related goal is to allow buffers mapped over NVM to be tracked
> by the existing monitoring and management APIs."
>
>> Description section
>>
>> It might be clearer of "Proposed Java API Changes" were renamed to
>> "Proposed JDK-specific API changes".
>
> Agreed.
>
>> One other thing to mention is that I re-read the javadoc for the MBB
>> force methods and I think we need to adjust one of the sentences in the
>> existing (and new) methods to take account of implementation specific
>> map modes. I've created JDK-8226203 [1] to track it. As support for
>> implementation specific map modes is in new in Java SE 13 then it might
>> be worth trying to get that fixed now while it is fresh in our minds.
> Sure, I'll post an RFR with the javadoc patch as a separate step. Can it
> go into jdk13? or is it too late for that?
>
> regards,
>
>
> Andrew Dinn
> ---
> Senior Principal Software Engineer
> Red Hat UK Ltd
> Registered in England and Wales under Company Registration No. 03798903
> Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Re: RFR (T): 8226203: MappedByteBuffer.force method may have no effect on implementation specific map modes
Hi Alan,
On 21/06/2019 12:34, Alan Bateman wrote:
> I saw Joe's comment on the CSR. This refinement looks good to me.
I have pushed the doc fix to jdk13.
Once it percolates to jdk14 I will post a new webrev for the JEP 352
implementation which accomodates this change. That webrev will also
include the changes pending after Joe's review of the implementation CSR
leaving (I hope) only my recently proposed JEP updates and endorsement
to finish before it can be pushed.
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Re: RFR (T): 8226203: MappedByteBuffer.force method may have no effect on implementation specific map modes
Hi Alan,
On 19/06/2019 14:11, Alan Bateman wrote:
> On 19/06/2019 11:07, Andrew Dinn wrote:
>> :
>> Do I still need to wait for confirmation for the CSR from Joe Darcy
>> before pushing to the jdk13 repo? (He already knows about the CSR).
>>
> Yes, anything that has a CSR needs to wait until it is approved.
Joe Darcy has approved the CSR. However, he suggested by way of code
review that the comment be tweaked to explicitly state that the force
method does have an effect when the buffer was mapped with mode
READ_WRITE e.g. instead of
* This method has no effect for buffers mapped in read-only
* or private mapping modes. It may also have no effect for other
* implementation specific map modes.
this
+ * If this buffer was not mapped in read/write mode
+ * ({@link java.nio.channels.FileChannel.MapMode#READ_WRITE})
+ * then invoking this method may have no effect. In particular,
+ * the method has no effect for buffers mapped in read-only or
+ * private mapping modes. This method may or may not have an
+ * effect for implementation-specific mapping modes.
Would you agree with that change or do you prefer to stick with the
original? If necessary I'll amend the patch and CSR then push whichever
version you prefer to JDK13.
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Re: RFR (T): 8226203: MappedByteBuffer.force method may have no effect on implementation specific map modes
On 19/06/2019 11:03, Alan Bateman wrote:
> I added myself as Reviewer to the the CSR so you can finalize. The
> webrev looks good.
Thanks, Alan. I have finalized the CSR.
Do I still need to wait for confirmation for the CSR from Joe Darcy
before pushing to the jdk13 repo? (He already knows about the CSR).
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Re: RFR (T): 8226203: MappedByteBuffer.force method may have no effect on implementation specific map modes
Hi Alan,
On 18/06/2019 18:08, Alan Bateman wrote:
> This looks good. Will you create a CSR for this? I think it can be fixed
> in jdk/jdk13 as it follows JDK-8221397 and JDK-8221696 (and there is no
> risk as it's javadoc only).
I raised this CSR:
CSR:https://bugs.openjdk.java.net/browse/JDK-8226385
and tagged it for jdk13.
Also, I labelled it SE -- but is it, perhaps, meant to be JDK?
(apologies, I am still a noob to this process).
Bug:https://bugs.openjdk.java.net/browse/JDK-8226203
Webrev: http://cr.openjdk.java.net/~adinn/8226203/webrev.00/
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
RFR (T): 8226203: MappedByteBuffer.force method may have no effect on implementation specific map modes
Could I please have a review for the following trivial change to the javadoc for the two MappedByteBuffer.force methods: JIRA: https://bugs.openjdk.java.net/browse/JDK-8226203 webrev: http://cr.openjdk.java.net/~adinn/8226203/webrev.00/ It allows for the possibility of failures when extended map modes are used to create the MappedByteBuffer. The case documented here will not actually arise in JDK13 since nothing in the JDK creates, let alone exposes extended map modes. However, it would be better if the javadoc reflected that possibility. Is it still possible to push this? If that is a problem then I am happy for it to go into jdk14. regards, Andrew Dinn ---
Re: RFR: 8224974: Implement JEP 352
Hi Alan,
Thanks for reviewing the JEP one more time.
On 16/06/2019 09:47, Alan Bateman wrote:
> I re-read the JEP, trying to put myself in the position of someone
> reading it for the first time in 2020.
>
> Summary section:
>
> What would you think about replacing this with a sentence that makes it
> clear that the JEP adds new JDK-specific file mapping modes to allow the
> FileChannel API create MappedByteBuffers over non-volatile memory?
I would be happy with that way of describing the behaviour change except
that what you suggest doesn't mention actually making it work -- which
/is/ part of the JEP. How about:
"Add new JDK-specific file mapping modes, allowing the FileChannel API
to be used to create MappedByteBuffers over non-volatile memory, and
extend the underlying implementation to support this new use case"
> Goals section:
>
> I think the first paragraph could be re-worded to make it clear that the
> goal is to use the existing MappedByteBuffer API to access NVM.
Yes indeed:
"This JEP proposes that class MappedByteBuffer be reworked to support
access to non-volatile memory (NVM). The only API change required is a
new enumeration employed by FileChannel clients to request mapping of a
file located on an NVM-backed file system rather than a conventional,
file storage system. Recent changes to the MappedByteBufer API mean that
it supports all the behaviours needed to allow direct memory updates and
provide the durability guarantees needed for higher level, Java client
libraries to implement persistent data types (e.g. block file systems,
journaled logs, persistent objects, etc.). The implementations of
FileChannel and MappedByteBuffer need revising to be aware of this new
backing type for the mapped file."
> Paragraphs 2-5 split this into two sub-goals. The first suggests that it
> extends the MBB API but this is no longer the case. The second also
> hints that it adds a new API. I think these two need to be re-worded.
Agreed. How about this:
"The primary goal of this JEP is to ensure that clients can access and
update NVM from a Java program efficiently and coherently. A key element
of this goal is to ensure that individual writes (or small groups of
contiguous writes) to a buffer region can be committed with minimal
overhead i.e. to ensure that any changes which might still be in cache
are written back to memory."
n.b. I snuck in the word coherence because I thought it clarified the
notion of committing to memory.
> Goal 3 and 4 are okay, although I think the 4th could be summarized as
> allowing mapped buffers on NVM to be tracked by the existing monitoring
> and management APIs.
Agreed. So, renumbering accordingly, how about this:
"A second, subordinate goal is to implement this commit behaviour using
a restricted, JDK-internal API defined in class Unsafe, allowing it to
be re-used by classes other than MappedByteBuffer that may need to
commit NVM.
A final, related goal is to allow buffers mapped over NVM to be tracked
by the existing monitoring and management APIs."
> Description section
>
> It might be clearer of "Proposed Java API Changes" were renamed to
> "Proposed JDK-specific API changes".
Agreed.
> One other thing to mention is that I re-read the javadoc for the MBB
> force methods and I think we need to adjust one of the sentences in the
> existing (and new) methods to take account of implementation specific
> map modes. I've created JDK-8226203 [1] to track it. As support for
> implementation specific map modes is in new in Java SE 13 then it might
> be worth trying to get that fixed now while it is fresh in our minds.
Sure, I'll post an RFR with the javadoc patch as a separate step. Can it
go into jdk13? or is it too late for that?
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Re: RFR: 8224974: Implement JEP 352
On 10/06/2019 18:44, Alan Bateman wrote:
> On 10/06/2019 11:09, Andrew Dinn wrote:
>> :
>> Finally, I am unclear whether the presence of the new module + package
>> means this is an SE JEP or a JDK JEP? In the latest update I have
>> assumed the JEP type is still JDK and changed the title for the public
>> API changes to "Proposed Java API Changes". Is that right? Or do you
>> want the JEP type to be SE and have the title remain "Proposed Java SE
>> API Changes"
>>
> The module is JDK-specific so keeping the scope "JDK" is right.
Ok, in that case I'll ask Brian if he can endorse the JEP and then
target for JDK14.
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Re: RFR: 8224974: Implement JEP 352
On 11/06/2019 00:30, Brian Burkhalter wrote:
> I wanted to let you know that I ran version 7 above through our tests
> and did not see any problems related to your changes.
Excellent. Thanks very much for running that check.
> I think the copyright years in PmemTest.java are wrong however: 2002,
> 2011. I suppose it should be just 2019.
Well spotted! Let's hope that is the final escapee.
I have tweaked my latest patch accordingly. I have also updated the
patch to specify @since 14 instead of @since 13 in the two places where
new elements are added. I'll post a final webrev once I am sure there
are no other changes needed.
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Re: RFR: 8224974: Implement JEP 352
Hi Alan, On 10/06/2019 09:18, Alan Bateman wrote: > I don't have any other feedback on the JEP beyond my last mail about > removing the "Proposed Java API Changes" section from the Description. > Once we you ready then Brian (as area lead) should be contacted to seek > his endorsement. I have updated the Proposed Java API Changes to remove the changes to map exception signature and force region specification. They were covered in the prior enabling JIRAs/CSRs. So, the remaining two sections mention 1) the new module + package + enum and 2) the bean counting changes. The new section 1 clarifies when UnsupportedOperationException is thrown vs when IOException is thrown as part of the explanation of what the new modes are for. I also added a paragraph to the alternatives section explaining that Panama was and still is an alternative option and why we decided to proceed with this model despite it being still under consideration. Finally, I am unclear whether the presence of the new module + package means this is an SE JEP or a JDK JEP? In the latest update I have assumed the JEP type is still JDK and changed the title for the public API changes to "Proposed Java API Changes". Is that right? Or do you want the JEP type to be SE and have the title remain "Proposed Java SE API Changes" regards, Andrew Dinn ---
Re: RFR: 8224974: Implement JEP 352
On 09/06/2019 16:53, Alan Bateman wrote:
> On 07/06/2019 12:24, Andrew Dinn wrote:
>> :
>> Modulo confirmation of those two checks this looks like it is a complete
>> implementation. Unless anyone has more changes to recommend?
>>
> I read through the recent mails and I think you are nearly done with the
> code review. There are still a few updates to be done to the JEP and it
> will need to be endorsed before propose-to-target. Given that JDK 13
> enters RDP1 on June 13 then I assume it's best to re-target the CSR to
> 14 and for the JEP to become a candidate for that release.
Yes, I agree it is too late to squeeze this into 13. I have updated the
fix version for the JEP and CSR to 14:
JEP: https://bugs.openjdk.java.net/browse/JDK-8207851
CSR: https://bugs.openjdk.java.net/browse/JDK-8224975
I'll wait for any further feedback on the JEP and implementation.
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Re: RFR: 8224974: Implement JEP 352
On 07/06/2019 12:24, Andrew Dinn wrote:
> On 06/06/2019 18:40, Vladimir Kozlov wrote:
>> Hotspot changes looks good.
>>
>> CheckGraalIntrinsics failed because of typo - you should use 'J' instead
>> of 'L':
>>
>> + "jdk/internal/misc/Unsafe.writeback0(L)V",
>
> Doh! Thanks, Vladimir :-)
>
> With that correction the test now passes.
>
>> One nitpick in vmSymbols.hpp spacing is off in next line:
>>
>> + do_intrinsic(_writebackPostSync0,
>> jdk_internal_misc_Unsafe, writebackPostSync0_name,
>> void_method_signature , F_RN) \
> That line and the one two lines further on were both misaligned.
>
> I have uploaded a new webrev to fix the bove problems. This version also
> removes all the extra extraneous whitespace found by Brian and Gustavo.
>
> webrev: http://cr.openjdk.java.net/~adinn/8224974/webrev.07/
>
> I have posted the changes to the submit repo to re-verify that all
> builds pass. I have also asked Jonathan Halliday to re-test this version
> against the Red Hat middleware clients to ensure there are no functional
> or performance changes.
>
> Modulo confirmation of those two checks this looks like it is a complete
> implementation. Unless anyone has more changes to recommend?
I just want to confirm that the submit job was clean and the middleware
clients are performing as expected.
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Re: RFR: 8224974: Implement JEP 352
On 06/06/2019 18:40, Vladimir Kozlov wrote:
> Hotspot changes looks good.
>
> CheckGraalIntrinsics failed because of typo - you should use 'J' instead
> of 'L':
>
> + "jdk/internal/misc/Unsafe.writeback0(L)V",
Doh! Thanks, Vladimir :-)
With that correction the test now passes.
> One nitpick in vmSymbols.hpp spacing is off in next line:
>
> + do_intrinsic(_writebackPostSync0,
> jdk_internal_misc_Unsafe, writebackPostSync0_name,
> void_method_signature , F_RN) \
That line and the one two lines further on were both misaligned.
I have uploaded a new webrev to fix the bove problems. This version also
removes all the extra extraneous whitespace found by Brian and Gustavo.
webrev: http://cr.openjdk.java.net/~adinn/8224974/webrev.07/
I have posted the changes to the submit repo to re-verify that all
builds pass. I have also asked Jonathan Halliday to re-test this version
against the Red Hat middleware clients to ensure there are no functional
or performance changes.
Modulo confirmation of those two checks this looks like it is a complete
implementation. Unless anyone has more changes to recommend?
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Re: RFR: 8207851: Implement JEP 352
On 06/06/2019 10:46, Andrew Haley wrote:
> Handy hint. In your .emacs, add this:
>
> ;;; goodbye trailing whitespace blues!!!
>
> (add-hook 'java-mode-hook
> (lambda ()
>(progn
> (set-variable 'show-trailing-whitespace t)
> )))
>
> (add-hook 'c-mode-hook
> (lambda ()
> (progn
> (set-variable 'show-trailing-whitespace t)
> )))
>
>
> (add-hook 'c++-mode-hook
> (lambda ()
> (progn
> (set-variable 'show-trailing-whitespace t)
> )))
There, fixed that for ya!
> ...and you'll never have any trouble with trailing whitespace again.
Now working beautifully, thank you!
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Re: RFR: 8207851: Implement JEP 352
Hi Gustavo,
On 05/06/2019 19:13, Gustavo Romero wrote:
> I found some trailing space in v5 and it seems they are in v6 as well.
Thanks for the heads up. I will fix these in the next webrev (07).
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Re: RFR: 8224974: Implement JEP 352
Hi Brian,
On 06/06/2019 01:06, Brian Burkhalter wrote:
> With version 6 [1] of the patch applied, tests throw this exception:
>
> Mapped java.lang.InternalError: java.lang.NullPointerException
> at
> java.base/jdk.internal.misc.ExtendedMapMode.newMapMode(ExtendedMapMode.java:58)
> at
> java.base/jdk.internal.misc.ExtendedMapMode.(ExtendedMapMode.java:40)
> at java.base/sun.nio.ch.FileChannelImpl.map(FileChannelImpl.java:1007)
>
> Looks like a problem in the initialization order of the static final
> constants: MAP_MODE_CONSTRUCTOR needs to be initialized first so I think
> the *_SYNC constants need to be after the static initializer [2].
Doh! Yes, you are right.
Sorry for adding confusion. webrev.06 was not meant to be intended for
public consumption. I uploaded it last night when I set the rebuild off
but I didn't intend to advertise it until I had retested and got your
confirmation on the suggestions I made. Anyway, thanks all the same for
reviewing it, trying to run it and also finding/fixing this issue.
I have updated webrev.06 in place and PmemTest now runs ok. I still need
to have Jonathan Halliday test the patch with our Transaction and Data
Grid software before it can be properly signed off. Another submit run
will also be needed. However, PmemTest is a good enough sanity check to
validate the changes you requested.
So, here is webrev.06 as the latest official patch:
http://cr.openjdk.java.net/~adinn/8224974/webrev.06/
It should address
changes requested on this list by Alan and Vladimir
all the changes you requested in your review
(modulo those you agreed to defer on)
the change to correct this static init order bug
a change to graal test CheckGraalIntrinsics to declare
the new jdk13 intrinsics in alphanum sort order
(this was requested offline by Alan -- without it the test
refuses to run on jdk13)
The last of those changes still leaves one issue unanswered (although it
may not actually be an issue). Vladimir is probably best able to comment.
When I run CheckGraalIntrinsics (which is actually run indirectly by
compiler/graalunit/HotspotTest) with my patched tree it fails, claiming
that Graal does not know about Unsafe.writeback0:
java.lang.AssertionError: missing Graal intrinsics for:
jdk/internal/misc/Unsafe.writeback0(J)V
I am assuming this is what is expected to happen? Or perhaps the test is
supposed to be added to the problem or exclude lists?
> Sorry for suggesting the change that was the proximate cause of this. :-(
No problem :-) Thanks for reviewing and checking!
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Re: RFR: 8224974: Implement JEP 352
Hi Brian,
Thanks very much for reviewing the patch. Also, commendations on your
eagle-eyed thoroughness.
I have happily accepted most corrections. I have commented (inline
below) in a few places where I think things might need to be discussed
further. I will upload a new webrev after these points are resolved.
On 05/06/2019 15:36, Brian Burkhalter wrote:
> I have some minor comments on webrev.05. (webrev.06 looks rather strange.)
That is the next working version being queued up -- you may well have
seen an intermediate version while it was uploading.
> Note: Hotspot changes not reviewed here.
Sure.
> General
> Check copyrights. Some headers are missing altogether, some have
> incorrect years.
Yes, all of them are now fixed.
> MappedByteBuffer.java
> 84:“any of other modes” -> “any of the other modes”, “this” -> “This”
> 85:nit: usually use American spelling: “behaviour” -> “behavior” (but it
> does not matter so much here as it is not public javadoc.
> 172:“pasing” -> “passing”
> 175:delete “using”
> 181:comma or semicolon before “otherwise”
All done. I even gritted my teeth and spelled behaviour 'US-wize'.
> 355-365: Collapse to?
>
> Unsafe.getUnsafe().writebackMemory(address + index, length);
Sure, done. Although personally I'm 'not much into that whole brevity
thing'.
> Unsafe.java
> 924-945:Capitalize first words of sentences.
Done.
> 959:Is there a possibility of overflow, e.g., use Math.addExact(address,
> length) ?
I think it assumed that all OSes currently ported (certainly Linux) will
never map a vmem region so that addition of a valid internal offset
might overflow. If it did we would be in big trouble as regards null
pointer checking. So, modulo input from those wiser than me in operating
system lore, I think this is not needed.
> 1290:Insert blank line after.
Done.
> FileChannelImpl.c (Unix)
> 85:“||! map_sync” -> “”|| !map_sync”
Done.
> 98-110:Put symbolic name definitions at head of file?
I thought these definitions would be better placed at the point where
the defines are needed so as to make it clear why this is being done. If
you are strongly motivated to move them to the more normal location I
will do so.
> FileChannelImpl.c (Windows)
> 91:Use same message as at Unix version line 135?
Perhaps but I don't think they are the same error and thougt it beter to
reinforce that with different messages. Note the different exception
types. In the case of Windows we should never reach this point -- at
least not until a Windows port comes along and requires the message to
be changed (effectively removing here and replacing it with somewhere
else). So, an InternalError is thrown. In the Unix case the error might
conceivably happen (because the target OS mmap implementation does not
have proper MAP_SYNC support. Hence the use of IOException.
> ExtendedMapMode
> 13:Constant name should be MAP_MODE_CONSTRUCTOR.
> 13:Insert blank line after
> 34-36:Move to before line 13.
> 24:Move constructor to end of class.
Yes, all good. Thanks!
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Re: RFR: 8224974: Implement JEP 352
On 04/06/2019 11:40, Alan Bateman wrote:
> On 03/06/2019 15:37, Andrew Dinn wrote:
>> :
>>
>> The CSR and JEP have been updated accordingly
>>
>> CSR: https://bugs.openjdk.java.net/browse/JDK-8224975
>> JEP: https://bugs.openjdk.java.net/browse/JDK-8207851
>>
> The specification section in the CSR was missing the module definition
> so I added that. The rest looks okay so I've added myself as Reviewer so
> you can finalize it.
Thanks, Alan. Done.
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Re: RFR: 8224974: Implement JEP 352
On 03/06/2019 14:17, Andrew Dinn wrote:
> Hi Vladimir,
>
> Thanks for the follow-up and for checking the submit failure. I have
> addressed all the points you raised (point by point comments are
> included below). A new webrev which passes a submit run is here:
>
> http://cr.openjdk.java.net/~adinn/8224974/webrev.04
>
> n.b. This webrev also includes changes to the javadoc for
> ExtendedMapMode suggested by Alan Bateman.
. . .
I have made one further change to locate the new ExtendedMapMode enum in
module jdk.nio.mapmode and package jdk.nio.mapmode as suggested offline
by Alan Bateman and the OpenJDK lead. Please refer to this webrev instead:
webrev: http://cr.openjdk.java.net/~adinn/8224974/webrev.05
The CSR and JEP have been updated accordingly
CSR: https://bugs.openjdk.java.net/browse/JDK-8224975
JEP: https://bugs.openjdk.java.net/browse/JDK-8207851
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Re: RFR: 8224974: Implement JEP 352
Hi Vladimir,
Thanks for the follow-up and for checking the submit failure. I have
addressed all the points you raised (point by point comments are
included below). A new webrev which passes a submit run is here:
http://cr.openjdk.java.net/~adinn/8224974/webrev.04
n.b. This webrev also includes changes to the javadoc for
ExtendedMapMode suggested by Alan Bateman.
regards,
Andrew Dinn
---
On 30/05/2019 17:58, Vladimir Kozlov wrote:
> On 5/30/19 9:08 AM, Andrew Dinn wrote:
>> I have uploaded a new webrev which attempts to address the problem.
>>
>> http://cr.openjdk.java.net/~adinn/8224974/webrev.03
>
> I looked only on HotSpot code.
Sure, thank you.
> stubGenerator_x86_64.cpp - in generate_data_cache_writeback() next are
> not used:
>
> + bool optimized = VM_Version::supports_clflushopt();
> + bool no_evict = VM_Version::supports_clwb();
Yes. I have removed them.
> vm_version_x86.hpp it should check CPUID flag in 32-bit:
>
> +#else
> + static bool supports_clflush() { return true; }
I have added a new feature flag CPU_FLUSH, which is set conditional on
the cpuid1 edx bit being set. The 32-bit version of supports_clflush()
test for the presence of this feature. The 64-bit version always returns
true. On 64-bit I still set the feature flag (unconditionally) even
though it is not used. I prefer the loss of 1-2 cycles to the risk of
surprising someone looking at the feature bits in a debugger.
> We don't check has_match_rule() in LibraryCallKit any more.
> In .ad files you need to add predicate to new insrtructions:
>
> predicate(VM_Version::supports_data_cache_line_flush());
Ok, done.
> Also add this check to Matcher::match_rule_supported() which you can use
> then in C2Compiler::is_intrinsic_supported().
Yes, this is much better. Done.
> DISABLE_UNSAFE_WRITEBACK_INTRINSIC should be checked much early may be
> together with os::supports_map_sync() when you set
> _data_cache_line_flush_size.
Doing that that would change the behaviour. If I push the check down
into the VM_Version code as you suggest then that will disable map and
writeback operations as well as the JIT intrinsic. This test was used to
allow the benefit of the intrinsic to be measured.
However, I don't think it is worth worrying about retaining this test.
It was useful during development but is probably not going to be needed
any more. So, I have removed it.
>> Unfortunately, this latest webrev still fails when uploaded to the
>> submit repo. ...
>
>
> t:/workspace/open/src/java.base/windows/native/libnio/ch/FileChannelImpl.c(64):
> error C2220: warning treated as error - no 'object' file generated
>
> t:/workspace/open/src/java.base/windows/native/libnio/ch/FileChannelImpl.c(64):
> warning C4029: declared formal parameter list different from definition
Ah, got it. The change to handle the isSync argument to map0 needs
propagating to the Windows native implementation.
Also, I tweaked the Windows C code to throw InternalError if mapSync is
ever passed as true. That /should never happen/ in the current
implementation. If/when this gets ported to Windows then the code that
does the throw can be replaced with case handling to call mmap with
MAP_SYNC.
RFR: 8224975: CSR: Implement JEP 352
Could I please have reviews for the following CSR which details the
changes needed for the JEP 352 implementation task:
CSR JIRA: https://bugs.openjdk.java.net/browse/JDK-8224975
I'm still hoping to target this for JDK13. The OpenJDK Project Lead
explained that this CSR needs to be reviewed with at least provisional
agreement before that can happen. Also, the JEP still needs endorsing by
at least one relevant Group or Area Lead (I think that probably means
Alan, Brian or Vladimir).
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Re: RFR: 8224974: Implement JEP 352
HI Vladimir,
Thank you for reviewing the patch.
On 29/05/2019 20:49, Vladimir Kozlov wrote:
> I tried to test these changes and build failed on all systems except
> Linux because:
>
> workspace/open/src/hotspot/share/prims/unsafe.cpp:446:3: error: use of
> undeclared identifier 'JNU_ThrowRuntimeException'
> JNU_ThrowRuntimeException(env, "writeback is not implemented");
> ^
Apologies for that. I forgot to test this via the submit repo after
cut-and-pasting the checks for OS and CPU support from the map0 native
method to the Unsafe writeback method.
I had to make some tweaks to this code anyway in order to spot an issue
Alan noticed when checking the CSR (the map code was not distinguishing
the precise cases where IOException and UnsupportedOperationException
needed to be thrown and would sometimes have replaced the latter with
the former on Windows/x86_64).
I have uploaded a new webrev which attempts to address the problem.
http://cr.openjdk.java.net/~adinn/8224974/webrev.03
The test to see whether writeback is enabled on the current cpu_os
combination is now performed in Java from methods Unsafe.writebackMemory
and FileChannelImpl.map, using a call to Unsafe.isWritebackEnabled()
There are also 'belt and braces' checks in the corresponding native
implementation methods:
Unsafe asserts that VM_Version::supports_data_cache_line_writeback()
returns true. The result of this method indicates whether support is
available on both CPU and OS. It returns a value computed using a call
to a new OS-specific method os::supports_map_sync() and, on hardware for
which that is true (AArch64 and x86_64), a test of the relevant CPU
status bits.
FileChannelImpl still relies on conditional compilation to reject calls
on invalid OS/CPU combinations (the VM_VERSION method is not available
for it to call). In the branch for !LINUX || !(AArch64 || amd64) it
throws an InternalError as this path not be reached.
Unfortunately, this latest webrev still fails when uploaded to the
submit repo. The problem seems to be specific to Windows/x86_64 builds.
The branch that failed is JDK-8224974-03. The returned text is appended
after my signature. Would you be able to provide some details about the
errors?
>
> Also Graal test should be fixed for new intrinsics (add them to
> 'toBeInvestigated' for isJDK13orHigher):
>
> src/jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.hotspot.test/src/org/graalvm/compiler/hotspot/test/CheckGraalIntrinsics.java
That has been fixed in the webrev mentioned above.
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
- 8< 8< 8< 8< 8< 8< ---
BuildId: 2019-05-30-1509485.adinn.source
No failed tests
Tasks Summary
UNABLE_TO_RUN: 18
NOTHING_TO_RUN: 0
KILLED: 0
EXECUTED_WITH_FAILURE: 4
FAILED: 0
HARNESS_ERROR: 0
NA: 0
PASSED: 55
Build
1 Unable to run
windows-x64-install-windows-x64-build-19 Dependency task
failed: mach5...1512-2804499-windows-x64-windows-x64-build-12
4 Executed with failure
windows-x64-windows-x64-build-12 error while building,
return value: 2
windows-x64-debug-windows-x64-build-13 error while building,
return value: 2
windows-x64-open-windows-x64-build-14 error while building,
return value: 2
windows-x64-open-debug-windows-x64-build-15 error while
building, return value: 2
Test
17 Unable to run
tier1-product-open_test_hotspot_jtreg_tier1_common-windows-x64-22
Dependency task failed:
mach5...1512-2804499-windows-x64-windows-x64-build-12
tier1-debug-open_test_hotspot_jtreg_tier1_common-windows-x64-debug-28
Dependency task failed:
mach5...804499-windows-x64-debug-windows-x64-build-13
tier1-debug-open_test_hotspot_jtreg_tier1_compiler_1-windows-x64-debug-31
Dependency
task failed: mach5...804499-windows-x64-debug-windows-x64-build-13
tier1-debug-open_test_hotspot_jtreg_tier1_compiler_2-windows-x64-debug-34
Dependency
task failed: mach5...804499-windows-x64-debug-windows-x64-build-13
tier1-debug-open_test_hotspot_jtreg_tier1_compiler_3-windows-x64-debug-37
Dependency
task failed: mach5...804499-windows-x64-debug-windows-x64-build-13
tier1-debug-open_test_hotspot_jtreg_tier1_compiler_not_xcomp-windows-x64-debug-40
Dependency task failed:
mach5...804499-windows-x64-debug-windows-x64-build-13
tier1-debug-open_test_hotspot_jtreg_tier1_gc_1-windows-x64-debug-43
Dependency task failed:
mach5...804499-windows-x64-debug-windows-x64-build-13
tier1-debug-open_test_hotspot_jtreg_tier1_gc_2-windows-x64-debug-46
Dependency task failed:
mach5...804499-windows-x64-debug-windows-x64-build-
Re: RFR: 8224974: Implement JEP 352
Hi Alan,
Apologies for the previous post which escaped from the lab while Dr
Funkenstein was struggling to push the right buttons (and work out what
happened when he pushed them).
I have created an implementation subtask and associated CSR. I also
updated the last webrev to record the javadoc changes necessitated in
order to complete the CSR. Finally, I set the JEP fix version to 13 and
pressed the big red "target" button.
Impl JIRA: https://bugs.openjdk.java.net/browse/JDK-8224974
CSR JIRA: https://bugs.openjdk.java.net/browse/JDK-8224975
webrev:http://cr.openjdk.java.net/~adinn/8224974/webrev.02/
n.b. I have switched to using the subtask JIRA id in $title and in the
cr.openjdk webrev link ...
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Re: RFR: 8224974: Implement JEP 352
Hi Alan,
I have created a new JEP implementation JIRA (note change to thread
title) and associated draft CSR
Impl JIRA: https://bugs.openjdk.java.net/browse/JDK-8224974
CSR JIRA:
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Re: RFR: 8207851: Implement JEP 352
Hi Alan,
Thank you for the review.
On 26/05/2019 19:25, Alan Bateman wrote:
> You may want to take a pass over the JEP to make sure that everything is
> accurate. I notice, for example, the section on BufferPoolMXBean has the
> old name READ_ONLY_PERSISTENT. We went through a couple of iterations in
> the discussion here so there might be a few others.
Thanks for spotting that. I corrected several wrong mentions of
PERSISTENT instead of SYNC. I also coerced a mention of MAP_SYNC to
render using a fixed width font.
I also updated the description of the exception signature changes to
FileChannel.map to explain how they will correlate with specific cases
handled by this JEP implementation (or, rather, cases that are
explicitly not handled by the implementation).
Finally I updated the MXBean name as you suggested (see below).
> I think the API changes are okay. I don't see a CSR yet but I assume
> you'll get to that soon.
Yes, I'll raise one ASAP. Could you clarify what changes I need to
document in the CSR? Here are my current thoughts:
ManagementFactoryHelper/FileChannelImpl
I am assuming the change that exposes the new MXBean needs to be
mentioned somwehere. However, that change doesn't actually affect any
API. It just means that a new bean with a new name appears in the list
of memory beans. I don't see anything which documents those bean names.
Am I missing something? (probably :).
com/sun/nio/file/ExtendedMapMode (in jdk.unsupported)
I'm assuming the CSR needs to propose javadoc for the 2 exposed MapMode
values, explaining what these modes are used for and which exceptions
documented in FileChannel.map get thrown for the cases where their use
is unsupported by the JVM or the OS, respectively. Is that correct?
jdk/internal/misc/ExtendedMapMode (in java.base)
Do I need to provide javadoc for the two new MapMode values and include
them in the CSR? I was assuming not.
FileChannelImpl method map
The javadoc in FileChannel lists the new exceptions that might be thrown
by this implementation but does not mention any specifics to say how
they might relate to use of the XXX_SYNC MapModes. Do I need to propose
updates for the FileChannel javadoc in the CSR or am I ok to provide
that detail in the doc for com/sun/nio/file/ExtendedMapMode?
Unsafe method writebackMemory
I was assuming Unsafe.writebackMemory is internal to the JDK so does not
need a mention in the CSR. Is that correct?
> I've read through the changes to java.base and jdk.unsupported.
>
> Just a few minor points:
>
> - I assume the update FileChannel.java should be dropped as it's just a
> left over from when we agreed to split out the updates to the Java SE API.
Yes, that is a leftover. It has been removed from the latest patch.
> - com.sun.nio.file.ExtendedMapMode.*_SYNC are missing javadoc, or rather
> the descriptions are truncated with "...". I think this dates from when
> were working out the right place to expose these constants. The source
> file (and the internal ExtendedMapModes are missing copyright headers too).
Thanks, I have added javadoc comments.
> - We didn't discuss the name of the buffer pool that is exposed through
> the JMX/management interface. We could take inspiration from the names
> of the CodeHeap spaces that are exposed with MemoryPoolMXBeans as there
> is an established convention for naming there, e.g. "mapped -
> 'non-volatile memory'".
The JEP used the name mapped_persistent" while the code named it
mapped_sync. I have changed both to use the name "mapped - 'non-volatile
memory'". Does this need further discussion by other parties? Or is that
a final decision?
> - Minor nit in Unmapper is that the methods to increment/decrement the
> usage should use Java conventions so probably should be incrementUsage
> and decrementUsage.
Caught me red-handed. Also fixed.
> - PmemTest. This is awkward and I wonder if it should be @run
> main/manual rather than @ignore. Also `@modules jdk.unsupported` would
> be useful to ensure it will be skipped if run with a test JDK that
> doesn't have this module.
Yes, I agree that @run main/manual is far better than @ignore and the
@modules requirement is also a very good idea. I have updated both.
New webrev: http://cr.openjdk.java.net/~adinn/8207851/webrev.01
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Re: RFR: 8207851: Implement JEP 352
Ping!
Any takers for a review?
Also, can anyone advise me on what I might need to do to target this JEP
to JDK13, other than the obvious reviewing and pushing of the
implementation?
regards,
Andrew Dinn
---
On 23/05/2019 11:55, Andrew Dinn wrote:
> Hi,
>
> Could I please have reviews of the following change set which implements
> JEP 352:
>
> JEP:https://openjdk.java.net/jeps/352
> JIRA: https://bugs.openjdk.java.net/browse/JDK-8207851
> webrev: http://cr.openjdk.java.net/~adinn/8207851/webrev.00/
>
> I would also very much like to target this implementation for JDK13.
>
> Testing:
>
> The webrev includes a simple test (in directory
> test/jdk/java/nio/MappedByteBuffer) which ensures that an NVRAM-backed
> MappedByteBuffer can be created, updated and forced using cache line
> flushes. This test is marked as ignored because it requires, inter alia,
> a suitably configured host, fitted with an NVRAM DIMM device or
> employing a pseudo-NVAM device simulated over volatile RAM.
>
> The above test has been run successfully on Linux x86_64 with an Optane
> DIMM and with a pseduo-NVRAM device. Further, more rigorous testing has
> been done in both the above configurations using the Narayana
> Transactions logger and Infinispan distributed data grid.
>
> Testing of /successful/ use of the API on Linux AArch64 has not yet been
> possible with either emulated or real NVRAM devices as it requires an
> updated (ARMv8.2) CPU hardware capability as well as access to AArch64
> compatible NVRAM devices. n.b. an AArch64 compatibility flag
> (-X:UsePOCForPOP) has been provided in the current patch to support
> testing on older CPUs using simulated NVRAM. Unfortunately, it has not
> yet been possible to obtain access to an AArch64 v8.1 machine that
> supports simulation of NVRAM devices via volatile RAM.
>
> In consequence, AArch64 testing has been limited to ensuring that the
> relevant API failure modes correctly manifest: i.e.
>
> v8.1 CPUs which lack the relevant hardware instructions refuse to map
> NVRAM-backed buffers trwoing UnsupportedOperationException
>
> v8.1 CPUs which bypass this failure via compatibility mode fail at the
> mmap stage with IOException due to lack of NVRAM mapping support in the
> underlying OS mmap API
>
> It is expected that the omissions in AArch64 testing will be rectified
> in the next few weeks. While this is desirable, the omissions are not
> viewed as critical since there is currently no general access to the
> relevant hardware.
>
> regards,
>
>
> Andrew Dinn
> ---
> Senior Principal Software Engineer
> Red Hat UK Ltd
> Registered in England and Wales under Company Registration No. 03798903
> Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
>
RFR: 8207851: Implement JEP 352
Hi,
Could I please have reviews of the following change set which implements
JEP 352:
JEP:https://openjdk.java.net/jeps/352
JIRA: https://bugs.openjdk.java.net/browse/JDK-8207851
webrev: http://cr.openjdk.java.net/~adinn/8207851/webrev.00/
I would also very much like to target this implementation for JDK13.
Testing:
The webrev includes a simple test (in directory
test/jdk/java/nio/MappedByteBuffer) which ensures that an NVRAM-backed
MappedByteBuffer can be created, updated and forced using cache line
flushes. This test is marked as ignored because it requires, inter alia,
a suitably configured host, fitted with an NVRAM DIMM device or
employing a pseudo-NVAM device simulated over volatile RAM.
The above test has been run successfully on Linux x86_64 with an Optane
DIMM and with a pseduo-NVRAM device. Further, more rigorous testing has
been done in both the above configurations using the Narayana
Transactions logger and Infinispan distributed data grid.
Testing of /successful/ use of the API on Linux AArch64 has not yet been
possible with either emulated or real NVRAM devices as it requires an
updated (ARMv8.2) CPU hardware capability as well as access to AArch64
compatible NVRAM devices. n.b. an AArch64 compatibility flag
(-X:UsePOCForPOP) has been provided in the current patch to support
testing on older CPUs using simulated NVRAM. Unfortunately, it has not
yet been possible to obtain access to an AArch64 v8.1 machine that
supports simulation of NVRAM devices via volatile RAM.
In consequence, AArch64 testing has been limited to ensuring that the
relevant API failure modes correctly manifest: i.e.
v8.1 CPUs which lack the relevant hardware instructions refuse to map
NVRAM-backed buffers trwoing UnsupportedOperationException
v8.1 CPUs which bypass this failure via compatibility mode fail at the
mmap stage with IOException due to lack of NVRAM mapping support in the
underlying OS mmap API
It is expected that the omissions in AArch64 testing will be rectified
in the next few weeks. While this is desirable, the omissions are not
viewed as critical since there is currently no general access to the
relevant hardware.
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Re: RFR (T): 8224042 Add private alignDown method to MappedByteBuffer
On 16/05/2019 17:27, Alan Bateman wrote:
> On 16/05/2019 15:49, Andrew Dinn wrote:
>> Please review this trivial change to MapperByteBuffer which encapsulates
>> the page align down operation in a suitably named method.
>>
>> JIRA: https://bugs.openjdk.java.net/browse/JDK-8224042
>> webrev: http://cr.openjdk.java.net/~adinn/8224042/webrev.00/
>>
> Looks okay (private static, as Thomas prefers, is fine too).
Thank you for the review, Alan.
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Re: RFR (T): 8224042 Add private alignDown method to MappedByteBuffer
On 16/05/2019 17:24, Mikael Vidstedt wrote:
>
> Looks good, thanks for doing it!
Thanks for the review Mikael.
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Re: RFR (T): 8224042 Add private alignDown method to MappedByteBuffer
Hi Thomas, Thanks for the review. On 16/05/2019 16:54, Thomas Stüfe wrote: > looks good and trivial. Method could be static. Yes, good point! I will modify accordingly and then push. regards, Andrew Dinn ---
Re: RFR : 8221696: MappedByteBuffer.force method to specify range
On 16/05/2019 14:14, Alan Bateman wrote:
> Mikael's suggestion seem okay. This is under the hood implementation so
> doesn't matter if there is a follow-on issue or you include it in the
> bigger patch. It might be easier to do the former while it's fresh in
> our minds.
Indeed. I have raised JDK-8224042, patched and tested the code, uploaded
a webrev and posted an RFR (T) to core-libs-dev. Reviews welcome!
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
RFR (T): 8224042 Add private alignDown method to MappedByteBuffer
Please review this trivial change to MapperByteBuffer which encapsulates
the page align down operation in a suitably named method.
JIRA: https://bugs.openjdk.java.net/browse/JDK-8224042
webrev: http://cr.openjdk.java.net/~adinn/8224042/webrev.00/
Testing:
This was successfully exercised by running
jdk/java/nio/channels/FileChannel/MapTest.java
The test rounds down a sequence of sub-page offsets to successive pages
within a mapped buffer during testing of MappedByteBuffer method force().
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Re: RFR : 8221696: MappedByteBuffer.force method to specify range
On 14/05/2019 10:57, Andrew Haley wrote: > On 5/13/19 5:14 PM, Andrew Dinn wrote: >> Thank you for looking at the patch. >> . . . >>firstly, that ps-1 clears the original bit and sets all lower bits; > > I think your core argument fails at this point. You have *already* > moved to considering a bitwise transformation as arithmetic. Hmm, yes. Well, at least that part doesn't drag in twos complement arithmetic, simply an understanding of how (the oh so very satisfying) rollover of all set bits happens when you add 1 to an all 1s pattern and hence how that rollover must happen in reverse when you subtract 1 from a single-bit pattern. > It helps a lot to think of the operation as sign extending the most > significant bit. This clears up the confusion and removes the need for > the obfuscation, I think. Yes, that's another interesting way to think about it. What is more interesting to me is how different programmers use a different base toolkit of concepts (and associated coding idioms) to convince themselves that specific combinations of logical and/or arithmetic operations achieve desired bit-twiddling outcomes. Of course, in many cases they don't -- because many programmers either simply don't encounter or carefully avoid bit-bashing problems. I suspect that, say, your work on compilers and, say, my work on graphics renderers taught us both many bit-twiddling tricks that are far from intuitive except when they are, if only just to us. > Still, it's not worth arguing about. You've at least thought about it. And perhaps not just post-hoc ;-) I have always coded and thought about rounding down this way having first seen the same idiom many years ago (in Interlisp-D code, believe it or not). regards, Andrew Dinn ---
Re: RFR : 8221696: MappedByteBuffer.force method to specify range
Hi Mikael,
Thanks for looking at this.
On 13/05/2019 17:41, Mikael Vidstedt wrote:
Would it be worth putting the logic in an aptly named (private) method?
Something like:
...
private long alignDown(long address, long alignment) {
return address & ;
}
...
long mapBase = alignDown(address, ps);
…
Yes, that would probably be an improvement. However, I am afraid the
ship has already sailed. I pushed the patch last night on the strength
of an ok from Alan posted after he tested the patch on Windows and
MacOS. Perhaps I should fix this in the push of the associated JEP
implementation?
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Re: RFR : 8221696: MappedByteBuffer.force method to specify range
On 26/04/2019 15:46, Alan Bateman wrote:
On 25/04/2019 17:34, Andrew Dinn wrote:
Also, here is a new webrev including the updated implementations for
mappingAddress/Offset/Length as described below
JIRA: https://bugs.openjdk.java.net/browse/JDK-8221696
webrev: http://cr.openjdk.java.net/~adinn/8221696/webrev.02
This looks right and I agree with your notes on negative addresses. As
its tricky to get right then it would be good to get another set of eyes
on this.
A minor nit is that we don't usually use underscores in variable names
here, it would be baseAddress rather than base_address, etc.
Alan tested this for me on Windows and MacOS with a successfl outcome
and oked the push in a private note. I pushed the patch yesterday --
modulo fixing the var names to use camelCase instead of under_score.
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Re: RFR : 8221696: MappedByteBuffer.force method to specify range
Thank you for looking at the patch.
On 28/04/2019 18:09, Andrew Haley wrote:
On 4/25/19 5:34 PM, Andrew Dinn wrote:
long map_base = (address & ~(ps - 1));
This looks like a hard way to write
long map_base = address & -ps;
My version certainly uses more characters, that is for sure. However, I
chose (and still prefer) this form for /clarity/. It seems to me to
communicate as simply as possible what mask is being constructed,
granted the starting premise that ps is a power of 2 (i.e. has only a
single bit set).
It only requires elementary knowledge of binary representations to see:
firstly, that ps-1 clears the original bit and sets all lower bits;
next that ~(ps-1) clears those lower bits and sets all bits from the
original (inclusive) upwards;
then that the result can be used as a mask to clear those same bottom
bits from address;
finally that this mask operation is equivalent to rounding down to
the relevant power of two.
I find your alternative (a tad) less clear because it employs an
arithmetic operation to achieve the requisite bitwise transformation.
That the two expressions compute to equivalent results requires some
experience and understanding of the twos-complement representations of
numbers rather than basic knowledge of bit fields.
As a gcc hacker 'your mileage may vary' ;-)
Crucially, every compiler we rely on is going to produce the same code
in both cases.
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Re: RFR : 8221696: MappedByteBuffer.force method to specify range
Also, here is a new webrev including the updated implementations for mappingAddress/Offset/Length as described below JIRA: https://bugs.openjdk.java.net/browse/JDK-8221696 webrev: http://cr.openjdk.java.net/~adinn/8221696/webrev.02 regards, Andrew Dinn --- On 23/04/2019 17:15, Andrew Dinn wrote: > Hi Alan, > > Thanks for looking at this. > > On 22/04/2019 16:52, Alan Bateman wrote: >> The calculation to compute the page aligned address and then the >> distance from there to the end of the region is tricky to get right. I >> think you have it right, I'm just wondering if we could leverage the >> existing mappingOffset/mappingAddress methods so that we have only one >> place to audit. For example, suppose mappingOffset is changed to take an >> index. I think this would reduce the new force method down to: >> >> long offset = mappingOffset(index); >> force0(fd, mappingAddress(offset), offset + length); > > I like this idea but I'd like to make sure I am clear about exactly how > it ought to work. If you will indulge me I will work through this by > describing first the status quo (as I perceive it) and then what my > change attempts before trying to reconcile the two. > > Currently: > > There are 4 key locations of interest in the mapped byte range > > [... map_base ... address ... address+limit ... address+capacity ...] > | | > +- page size -+ > > where: > - addresses to the left are numerically (i.e. as longs) lower than > those to the right > - map_base is aligned to page size -- it is obtained by rounding down > address to a page boundary > - consequently (address - map_base) is less than page size > > n.b. the mmapped region may or may not commence at map_base but it will > always include map_base. Also, it will include bytes at all subsequent > addresses up to (address + capacity). Whether it includes or extends > beyond (address + capacity) is not really important. > > mappingOffset() computes (address - map_base): > > int ps = Bits.pageSize(); > long offset = address % ps; > return (offset >= 0) ? offset : (ps + offset); > > The jiggery-pokery with ? : works round the rather unhelpful behaviour > of mod wrt negative longs. > > I think it could, equivalently, compute it directly as: > > int ps = Bits.pageSize(); > long map_base = (address & ~(ps - 1)); > return address - map_base; > > mappingAddress(mappingOffset) converts a previously retrieved > mappingOffset back to the map base: > >return (address - mappingOffset) > == (address - (address - map_base)) > == map_base > > > mappingLength(mappingOffset) computes ((address + capacity) - map_base) > i.e. the byte count from start of page map_base to the last > (potentially) /writeable/ buffer byte. > > return capacity() + mappingOffset > == capacity + (address - map_base) > == (address + capacity) - map_base > > arguably this method could just compute ((address + length) - map_base) > i.e. the byte count from map_base to the last /written/ byte -- but that > makes no real difference when performing a file-based flush. > > The call to force0 is made as > > force0(fd, mappingAddress(offset), mappingLength(offset)) > > which is equivalent to > > force0(fd, map_base, capacity + (address - map_base)) > > > My updated code: > > When looking at an indexed location there are now 7 key locations of > interest in the mapped byte range > > [... m_base ... a ... i_base ... a+i ... a+i+ln ... a+lim, a+cap ...] > | | | | > +- page size -+ +- page size -+ > > > where: > - for brevity, map_base is abbreviated to m_base, address to a, > index_base to i_base, index to i, length to ln, limit to lim > and capacity to cap > - index is identifies the start of the region to be flushed > - length is the length of the region to be flushed > - (address + index) is the address of the byte at offset index > - index_base is the largest page aligned address below (address + idx) > i.e. it is obtained by rounding down the first byte of the flush > region to a page boundary > > index_base is computed as follows > > int ps = Bits.pageSize(); > long index_base = ((address + index) & ~(ps - 1)); > > My code is calling > > force0(fd, index_base, length + ((address + index) - i_base)) > > i.e. it the flushed range starts at the nearest page boundary below or > equal to (address + index) and includes the original length bytes plus > the intervening bytes between that page boundary and address. > >
Re: RFR : 8221696: MappedByteBuffer.force method to specify range
ess = address + index;
return index_address - mappingOffset
== (index_address - (index_address - index_base))
== index_base
}
mappingAddress(mappingOffset) => mappingAddress(mappingOffset, 0)
mappingLength(mappingOffset, length)
{
return length + mappingOffset
}
mappingLength(mappingOffset) =>
mappingLength(mappingOffset, (long)capacity())
Does that look correct? If so I will prepare a new webrev accordingly.
> I don't expect any issues on Windows but I will test it to make sure.
Thank you. That would be very helpful.
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Re: RFR : 8221696: MappedByteBuffer.force method to specify range
Hi Alan, The CSR for this issue has been finalized and is awaiting approval. Would it now be possible to proceed with reviews of the implementation? JIRA: https://bugs.openjdk.java.net/browse/JDK-8221696 webrev: http://cr.openjdk.java.net/~adinn/8221696/webrev.01 regards, Andrew Dinn ---
Re: RFR : 8221696: MappedByteBuffer.force method to specify range
On 11/04/2019 11:34, Alan Bateman wrote:
> On 10/04/2019 12:15, Andrew Dinn wrote:
>> :
>> An updated webrev is available:
>>
>> JIRA: https://bugs.openjdk.java.net/browse/JDK-8221696
>> webrev: http://cr.openjdk.java.net/~adinn/8221696/webrev.01
>>
>>
> The updated javadoc looks good. A minor nit is that it reads "and
> includes {@code length} bytes" when it should say "and is {@code length}
> bytes". There is also a stray quote at the end of the paragraph.
Ok, I have corrected that and will update the patch accordingly
> I've added myself as Reviewer to the CSR so you can finalize it.
Excellent, thank you. It has now been finalized.
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
RFR: CSR: JDK-8222261: MappedByteBuffer.force method to specify range
Could I please get a review for the following CSR which accompanies
JDK-8221696
JIRA: https://bugs.openjdk.java.net/browse/JDK-861
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Re: RFR : 8221696: MappedByteBuffer.force method to specify range
On 09/04/2019 18:59, Alan Bateman wrote:
> There are a couple of implementation details to discuss but I'll stick
> to the API/javadoc for now so that you can create the CSR.
Ok, thanks.
> We discussed the method signature, parameter types, and the semantics in
> previous mails so I think they are all okay (meaning the first parameter
> is the index in the buffer of the first byte to write back and not an
> offset from the position). One nit is that the existing absolute methods
> name the parameter "index" and I think we should do the same here.
Yes, that is clearer as well as consistent.
> In the first sentence it might be better to say "a region" rather "some
> region". You could expand this paragraph with a second sentence,
> something like "The region starts at the given {@code index} in this
> buffer and is {@code length} bytes".
Ok.
> A suggestion for the description for the index and length parameters is
> copy the wording/style from the existing absolute methods, e.g.
>
> @param index The index of the first byte of the regsion; must be
> non-negative and less than limit()
> @param length The length of the region in bytes; must be non-negative
> and no larger than limit() - index
> @throws IndexOutOfBoundException If the preconditions on the index and
> length do not hold.
Ok.
An updated webrev is available:
JIRA: https://bugs.openjdk.java.net/browse/JDK-8221696
webrev: http://cr.openjdk.java.net/~adinn/8221696/webrev.01
I will also create a CSR based on the changes in this webrev and post an
RFR.
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Re: RFR : 8221696: MappedByteBuffer.force method to specify range
On 09/04/2019 19:15, Brian Burkhalter wrote:
>
>> On Apr 9, 2019, at 10:59 AM, Alan Bateman > <mailto:alan.bate...@oracle.com>> wrote:
>>
>> There are a couple of implementation details to discuss
>
> I was wondering whether this topic has any synergy / overlap with this
> prior thread [1] the principal sticking point of which was IIRC forcing
> / loading a slice of a MBB.
>
> http://mail.openjdk.java.net/pipermail/nio-dev/2019-February/005871.html
Oh, bonus points for that question ;-)
Currently, for file-backed MBBs, forcing a slice of the MBB has no
effect. That happens because the slice is not provided with an fd when
it is cloned off the MBB. The same applies when creating a view over the
byte buffer memory using some other type e.g. as an int buffer. I don't
know exactly why that decision was made (avoiding profileration of fds?)
but anyway it is a fait accompli and it means you can only force writes
to a slice or view using the original MBB.
When creating a slice (or view) over an NVRAM-backed buffer the
situation is different. It would be perfectly possible for the
slice/view to record that it's originating MBB is backed by NVRAM
without needing to store the fd in the slice/view. It does store the
base address of the originating MBB and its own offset/scaling. So, it
could easily translate the force indices in its own coordinate system
into a corresponding region of the original byte buffer and then write
back the relevant cache lines in the original MBB byte address range.
Much as the above capability would be useful it would mean that
NVRAM-backed buffer slices/views had different semantics to file-backed
backed buffer slices/views. The prototype implementation I provided
earlier chose to follow the existing behaviour so as not to open up any
such difference.
I am interested to hear if anyone thinks the alternative choice is worth
pursuing. It would certainly make life easier for users of NVRAM-backed
buffers as it would enable them to use the new flush API to flush
regions using slice/view coordinates rather than translated coordinates
for the original byte buffer. However, providing this capability only
for NVRAM-backed buffers leaves the solution somewhat half-baked.
No doubt, existing users of file-backed MBB slices/views have already
worked round the current limitation by retaining a handle on the
original MBB and flushing it at suitable points using a full force()
call. However, those existing implementations will not have had to deal
with coordinate translation since the flush is indiscriminate. If they
wish to move to using this new region-based flush then they will have to
record and translate buffer coordinates. I would prefer this requirement
to be fully present or fully removed rather than imposing it for one
case but not the other.
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Re: RFR : 8221696: MappedByteBuffer.force method to specify range
On 09/04/2019 19:30, Andrew Haley wrote:
> On 4/9/19 11:42 AM, Andrew Dinn wrote:
>> This new API method was conceived as a preliminary change for JEP 352 to
>> allow selective writeback of NVRAM-backed buffers. However, it has been
>> implemented to provide a similar capability for file-mapped byte
>> buffers. The old brute-force API method, force(), continues to operate
>> as before for file-mapped byte buffers.
>>
>> One detail that is worth highlighting is that for file-backed buffers
>> the start address passed to the native method force0 is rounded down to
>> a page boundary. This is needed for Unix implementations to ensure that
>> the underlying msync system call does not throw an exception.
>
> Is it actually necessary to use a system call to do this? I would have
> thought that NVRAM allowed a finer granularity than a whole page, too.
It is necessary to use a system call to write back a normal, file-backed
buffer, whether that is for the whole buffer (original force() method)
or some subset of it (new force(int,int) method). That case is all that
the current patch is concerned with.
When NVRAM support is added the force operation will, of course, employ
a cache line writeback and will only do so for the lines that overlap
the region (i.e. it will operate at cache-line granularity rather than
file page granularity).
The prototype implementation of NVRAM support posted in earlier
discussions shows how to do that, including how to ensure that the
writeback is done using an inlined machine instruction.
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Re: RFR: 8221397 Support implementation-defined Map Modes
On 09/04/2019 17:49, Alan Bateman wrote: > On 09/04/2019 17:02, Andrew Dinn wrote: >> In response to Alan's suggestion (included below) I have reverted the >> constructor for MapMode to private and will use behind the scenes access >> to construct the extended enum values. >> >> This change removes the need to modify the test in MapTest.java (no new >> API to exercise). >> >> It still requires javadoc changes in FileChannel.map and implementation >> changes in FileChannelImpl.java. Reveiews for the udpated webrev would >> be welcome. I'll post a separate request for review of the CSR changes. >> >> JIRA: https://bugs.openjdk.java.net/browse/JDK-8221397 >> new webrev: http://cr.openjdk.java.net/~adinn/8221397/webrev.03 >> > I think this looks fine. I've added myself as Reviewer to the CSR too. Thank you, Alan. Does that mean it is ok to push this now or do I need a second reviewer? regards, Andrew Dinn ---
Re: RFR: 8221397 Support implementation-defined Map Modes
In response to Alan's suggestion (included below) I have reverted the
constructor for MapMode to private and will use behind the scenes access
to construct the extended enum values.
This change removes the need to modify the test in MapTest.java (no new
API to exercise).
It still requires javadoc changes in FileChannel.map and implementation
changes in FileChannelImpl.java. Reveiews for the udpated webrev would
be welcome. I'll post a separate request for review of the CSR changes.
JIRA: https://bugs.openjdk.java.net/browse/JDK-8221397
new webrev: http://cr.openjdk.java.net/~adinn/8221397/webrev.03
Testing:
Submit repo: in progress
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
On 09/04/2019 13:47, Alan Bateman wrote:
> What would you think about not promoting the constructor to public? The
> original motivation for that was to be able to create additional map
> modes in jdk.internal.misc.ExtendedMapMode but we can use shared secrets
> or other skullduggery to do that. The advantage is that we avoid
> creating expectation in the API that developers can create their own map
> modes. The rest of the solution doesn't change, it's just eliminating
> the ability to create modes beyond the standard and extended modes.
Re: RFR: 8222107 (CSR) Support implementation-defined Map Modes
On 09/04/2019 10:09, Andrew Dinn wrote:
> Could I please get a review for the following CSR. It relates to the
> change proposed in JDK-8221397 which enables FileChannel
> implementation-specific extensions to enum MapMode.
>
> https://bugs.openjdk.java.net/browse/JDK-8222107
I have updated this CSR to fully detail all the javadoc changes required
in FileChannel.impl.
Following Alan Bateman's latest review of the implementation patch there
is no need to make the MapMode constructor public. So the only changes
required for this CSR are to javadoc.
Reviews would be welcome.
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Re: RFR : 8221696: MappedByteBuffer.force method to specify range
On 09/04/2019 16:04, Daniel Fuchs wrote:
> . . .
> That reads fine, thanks!
Good. Thanks for reviewing this.
> BTW: I haven't really looked at the implementation, I'm leaving that to
> the experts of the field :-)
No problem. I await their expert judgement.
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Re: RFR: 8221836: Avoid recalculating String.hash when zero
On 09/04/2019 13:21, Claes Redestad wrote:
> On 2019-04-09 11:05, Andrew Dinn wrote:
>> It would probably also be good if you extended the comment to document
>> the status quo i.e. as Peter noted that the assigned values are computed
>> deterministically from immutable state.
>
> How about this:
> http://cr.openjdk.java.net/~redestad/8221836/open.03/
Yes, that looks fine.
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Re: RFR: 8221397 Support implementation-defined Map Modes
On 09/04/2019 13:47, Alan Bateman wrote:
> On 08/04/2019 15:05, Andrew Dinn wrote:
>> :
>> Hmm, yes that's an interesting point. Of course, I'll take whatever
>> suggestions you are willing to throw this way :-)
>>
> What would you think about not promoting the constructor to public? The
> original motivation for that was to be able to create additional map
> modes in jdk.internal.misc.ExtendedMapMode but we can use shared secrets
> or other skullduggery to do that. The advantage is that we avoid
> creating expectation in the API that developers can create their own map
> modes. The rest of the solution doesn't change, it's just eliminating
> the ability to create modes beyond the standard and extended modes.
I'm happy to skulldig if you provide the advice on how to do it (an
Idiot's Guide to Trepanning?). It definitely sounds better to expose
this in a controlled way rather than via a general API.
Can I assume this will remove the need for a CSR?
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Re: RFR : 8221696: MappedByteBuffer.force method to specify range
Hi Daniel,
Thanks for looking at this.
On 09/04/2019 12:28, Daniel Fuchs wrote:
> Hi Andrew,
>
> On 09/04/2019 11:42, Andrew Dinn wrote:
>> One detail that is worth highlighting is that for file-backed buffers
>> the start address passed to the native method force0 is rounded down to
>> a page boundary. This is needed for Unix implementations to ensure that
>> the underlying msync system call does not throw an exception.
>
> Maybe this should be highlighted in the API documentation too,
> possibly as a non-normative implementation detail - stating
> that an implementation is free to do this (e.g. in an
> @implNote).
>
> My reading of your current proposed specification is that
> `from` is
> 234 * The offset to the first byte in the buffer region that
> 235 * is to be written back to storage
>
> and well - if I'm not mistaken then it appears the implementation
> can write some bytes before `from`, and that would be observable
> if you compared the file before and after calling force, isn't it?
There is no implication in the current documentation that a call to
force will /only/ write back bytes in the affected region. However, I
agree that it should be stated explicitly that this may happen.
I am not sure that this needs to be mentioned in an implNote. It is of
the nature of most memory-mapped storage devices that writeback has a
minimum granularity well above byte level. Would you be ok with a
correspondingly general caveat?
For example, what if I changed the second paragraph in the commment to:
* If the file mapped into this buffer resides on a local
* storage device then when this method returns it is guaranteed
* that all changes made to the selected region of the buffer since
* it was created, or since this method was last invoked, will have
* been written to that device. The force operation is free to
* write bytes that lie outside the specified region, for example
* to ensure that data blocks of some device-specific granularity
* are transferred in their entirety.
*
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
RFR : 8221696: MappedByteBuffer.force method to specify range
Could I please get reviews for the following patch which overloads
MappedByteBuffer.force to accept a start offset and length.
JIRA: https://bugs.openjdk.java.net/browse/JDK-8221696
webrev: http://cr.openjdk.java.net/~adinn/8221696/webrev.00
This new API method was conceived as a preliminary change for JEP 352 to
allow selective writeback of NVRAM-backed buffers. However, it has been
implemented to provide a similar capability for file-mapped byte
buffers. The old brute-force API method, force(), continues to operate
as before for file-mapped byte buffers.
One detail that is worth highlighting is that for file-backed buffers
the start address passed to the native method force0 is rounded down to
a page boundary. This is needed for Unix implementations to ensure that
the underlying msync system call does not throw an exception.
I am not sure whether Windows imposes this same restriction. If not then
it might be better to perform the rounding in the native code. Advice
would be welcome.
Testing:
I have only tested the new functionality on Linux. Assistance testing
this on other OSes would be appreciated.
[n.b. it should be enough to build with the patch and run
$ make test TEST=test/jdk/java/nio/channels/FileChannel/MapTest.java"
]
Updated MapTest.java: passes on Linux
Submit Test: in progress
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Re: RFR: 8222107 (CSR) Support implementation-defined Map Modes
Could I please get a review for the following CSR. It relates to the
change proposed in JDK-8221397 which enables FileChannel
implementation-specific extensions to enum MapMode.
https://bugs.openjdk.java.net/browse/JDK-8222107
Thanks.
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
