JDK-8257827 : C1/C2 compiler support for blackholes
  • Type: CSR
  • Component: hotspot
  • Sub-Component: compiler
  • Priority: P4
  • Status: Closed
  • Resolution: Withdrawn
  • Fix Versions: 16
  • Submitted: 2020-12-07
  • Updated: 2021-01-06
  • Resolved: 2021-01-06
Related Reports
CSR :  
JDK-8252505 - C1/C2 compiler support for blackholes
Relates :  
JDK-8257848 - -XX:CompileCommand=blackhole,* should be diagnostic
Relates :  
JDK-8258619 - -XX:CompileCommand=blackhole,* should only accept empty static methods
Relates :  
JDK-8258558 - Revert changes for JDK-8252505 and related issues
Description
(This is the retroactive CSR request, after the post-integration questions raised about it. Neither me or reviewers anticipated them at the time of review. Apologies for not catching that earlier. We can still change the implementation if CSR questions need to be resolved. See the start of discussion here: https://github.com/openjdk/jdk/pull/1203#issuecomment-739577111)

Summary
-------

Add a special support for so called "blackholes", the methods that are specially treated as having one special effect, and one effect only: keeping the arguments alive. This is important for the reliable and precise nano-benchmarking. Blackholes are defined with explicit HotSpot JVM flag `-XX:CompileCommand=blackhole,<method>`.

Problem
-------

The ability to avoid dead-code elimination is critical for benchmarking. JMH achieves this by providing a Java-based `Blackhole.consume` method. It unfortunately comes with several major performance drawbacks: call costs dominate nanobenchmarks, the work spent in `Blackhole.consume` dominates nanobenchmarks too, argument preparation for call makes different argument types behave differently. Compiler support for Blackholes is very useful for nano-benchmarks, as demonstrated by the experiments.

Solution
--------

Current C1/C2 JIT compilers implementation is able to receive the "blackhole" command from the user, and inline the method with removing its body, but keeping the incoming arguments alive.  The CSR-related question is how to communicate that command to JVM. 

Current implementation reuses the `CompilerOracle` facilities, available via `-XX:CompileCommand` HotSpot JVM flag. This eliminates the need to specify the public Java API, and also limits the accesibility/exposure of the new feature. JMH is able to use this feature by explicitly enabling it with `-XX:CompileCommand=blackhole,org.openjdk.jmh.infra.Blackhole::consume`. JMH already uses a lot of `CompileCommands` to ask for unusual things from the Hotspot JVMs, like inline/dontinlne/exclude, etc.

The upside of exposing the functionality as a command-line option is it has much lower level of commitment associated with it. If there's enough motivation/justification to graduate it into public Java API, it can be done later when there's enough confidence that it's the right fit there. See the "Alternatives" for more discussion.

This CSR comes from two points:

 1. **`CompileCommand` is the `product` flag.** Therefore, the change introduces a new behavior to the externally available `product` flag. Arguably, the spirit of the flag is `diagnostic`/`experimental`, but it's "letter" is `product`, due to historical reasons. [This is resolved by adopting Alternative 1, JDK-8257848]

 2. **`-XX:CompileCommand=blackhole,<method>` changes the semantics of the target method by eliminating its body.** The only other `-XX:CompileCommand=*` that does intrusive changes like this is `BreakAt`, which stops execution at method entry. This looks like low risk, as `-XX:CompileCommand` is the low-level compiler request, and we should expect users know what they are doing when requesting it (likewise for `BreakAt`). Similarly, the user controlling the HotSpot JVM invocation line can Xbootclasspath/patch-module the code out with similar semantics changing effects. [This can be resolved by adopting Alternative 2, JDK-8258619]

Alternatives
--------

**Alternative 1**. [Adopted with JDK-8257848]. Require `-XX:+UnlockDiagnosticVMOptions` when using `-XX:CompileCommand=blackhole,<method>`. I believe it would make the change under discussion "experimental", and thus it would fall out of scope for CSR. It would require minimal adjustments to VM implementation, VM tests, JMH-side support.
 
**Alternative 2.** [Prototyped as JDK-8258619]. Limit "blackholing" to the empty methods. Empty methods have no expected side effects, thus method body is not "eliminated", and there is no direct behavioral change. It is still a performance change in the sense that non-blackholed empty method can still be inlined and its inputs dead-code eliminated. However, this comes with major drawbacks in testing, as figuring out the method is indeed blackholed is much harder without directly observable elimination effects -- current jtreg tests assert blackholing works by testing method body is gone. Plus, it has the major drawback in JMH, where using such VM facility would require significant re-engineering of Blackhole support in JMH itself, probably introducing much more intrusive JDK-specific hacks and risks introducing methodological bugs in already intricate code in JMH.

**Alternative 3.** Remove additions to `-XX:CompileCommand`, and instead intrinsify the JMH methods directly. Unfortunately, this goes against the formal requirement that JVM only intrinsifies the "trusted" code from within the JDK, and thus intrinsifying 3rd-party code is unlikely to be accepted by VM teams. That said, this is the strategy that Graal uses today:
https://github.com/oracle/graal/blob/744654c108178d531c99fc802f451b55b72a48c9/compiler/src/org.graalvm.compiler.replacements/src/org/graalvm/compiler/replacements/StandardGraphBuilderPlugins.java#L1492.

**Alternative 4.** Introduce `Unsafe.blackhole` methods, and intrinsify them. Unfortunately, benchmark harnesses like JMH are the 3rd party code as far as JDK is concerned, which means using them requires linking to either `sun.misc.Unsafe` (which would extend the surface of "public" Unsafe, that we are unwilling to do), or `jdk.internal.vm.Unsafe` (which would require breaking in with elevated module system privileges).

**Alternative 5.** Introduce `blackhole` methods in `Whitebox` API. Unfortunately, the experience with Whitebox API in jcstress (which uses it to gain access for deoptimization) tells that linking against frequently changing `Whitebox` is very tedious -- the interface there is too fluid. Additionally, this would require intrinsifying the `Whitebox` methods themselves, which again raises the question about intrinsifying the methods formally outside of JDK (while there are native methods defined in JVM in ./src/hotspot/share/prims/whitebox.cpp, Whitebox Java entries are in ./test/lib/sun/hotspot/WhiteBox.java).

**Alternative 6.** Introduce public methods in the platform APIs. As stated in the Solution, this does not look appealing at this phase in compiler support work. Not overexposing the blackhole support allows wider testing with JMH, while retaining the right to retract it at any time if problems show up. Rushing to introduce the public API at this point would risk introducing the problematic API before understanding all the problems with its implementation.

Specification
-------------

There is no formal specification for `-XX:CompileCommand` option, the change adds new sub-behavior in product macro-flag.
Comments
Given the reversion of the commit (https://git.openjdk.java.net/jdk16/commit/ad456787), I'm moving this CSR to Closed - Withdrawn.
06-01-2021
As the result of CSR discussion, we are reverting the original commit and related issues from JDK 16 with JDK-8258558, to reconsider it for JDK 17.
05-01-2021
The additional alternatives are good thoughts. Some quick comments: > Alternative 4. Introduce Unsafe.blackhole methods, and intrinsify them. Unfortunately, benchmark harnesses like JMH are the 3rd party code as far as JDK is concerned, which means using them requires linking to either sun.misc.Unsafe (which would extend the surface of "public" Unsafe, that we are unwilling to do), or jdk.internal.vm.Unsafe (which would require breaking in with elevated module system privileges). I'm not sure "unwilling to extend s.m.Unsafe" is the right characterization here. The dynamics surrounding Unsafe are tricky; it was initially envisioned as a form of "preview" holder for low-level APIs, but like many things, it got abused. Assuming that this will ultimately graduate to a public API, then some sort of Unsafe-like incubating ground is not out of the question. > Alternative 6. Introduce public methods in the platform APIs. As stated in the Solution, this does not look appealing at this phase in compiler support work. We now have mechanisms for introducing APIs that are not final, such as "Preview". If the intention is that there will be a public API, though we're not 100% sure what it is, then Preview might well be your ticket here. Yes, you have to say `--enable-preview` to access it, but this is a small intrusion for an offline activity, and likely to be temporary (once the feature exits preview, the flag is no longer needed.)
18-12-2020
To make a discussion more pointed, I amended the CSR test with a few more alternatives that floated up in this discussion, and made it "Finalized" again.
18-12-2020
Brian, yes, I believe it is digressing into the wrong thing, so let me reiterate: the *actually important part is the compiler implementation*. _That_ is a design problem, and it was discussed at length. It is hard to get right and clean (I did 4 iterations of C2 code, every time being completely sure I did it right!), you get interesting failure modes if you implement it incorrectly. This is why real-world experimental testing is important. The public interface part is important for future too, but it is a second-, or even third-order concern here. In fact, *not overexposing the blackhole support* is the key to collect the data how it works or does not work, which has the important implications to _any future API discussion_. It *is* the right thing to do: get it tried on real-life cases while retaining the right to retract it. In other words, let's not be focusing on a wrong thing at this time. It is too early to have pubilc API discussion: we need to understand how exactly this thing _works_, without paying undue attention on how it _looks_, given it looks similar to what we already have (compiler commands). That is to say, current work makes blackhole support available for testing, and future work can (and should) discuss if there is a safe way to expose this at _platform level_. This CSR _is_ the venue to discuss if there are compatibility problems with current work, given the low commitment we are after. To address your concerns, in order: 1. Conformance issue (and discussion whether it is a conformance issue...) seems to be taken care of with Alternative 2. 2. Designating the option as "diagnostic" looks non-problematic to me, and it actually gives us a way to retract this work, in the unlikely case it would run into irreconcilable troubles in real-world JMH usages. We can turn that suboption back to "product", if you want, but it would rob us of the opportunity to ditch it in a single release. I would like to have that safety net. 3. Current CompilerCommand interface matches what JMH is already using to get non-standard things from the JVM: i.e. the same CompileCommands JMH issues en-masse for its infra and benchmark code. It is not something we came up on a whim. It is already the low-level Hotspot-specific interface that is used, that is tried, that has interface kinks worked out for those special you-know-what-you-are-doing uses.
17-12-2020
I get that you are anxious to put this to bed, and that's natural. I get also why you want to take the prior agreement from the compiler folks as definitive. But I think this discussion is digressing into the wrong thing, which is "what is stopping me from pushing before the 16 deadline." The right thing to be discussing is: "what's the right thing to do, for the long-term good of the platform." Ignoring the digressions, what the CSR review is revealing is that the pre-CSR design discussion should have been more extensive. No problem, it happens. (The CSR should have also preceded the integration, but that happened honestly too.) This is all correctible. To be specific, I have raised at least these three concerns: - the current mode of exposure has conformance issues (you have proposed a possible approach for this); - the designation as a "diagnostic" option is ... questionable; - even if the conformance issues are addressed, it is not clear to me that trying to sneak this in via a compilation flag is the right way to expose it, and I don't think enough examination of alternatives interface approaches (such as public API, or Unsafe) was done. Together, these say to me that "the design discussion isn't yet complete." Which happens all the time; it is common to think we are 100% done when we're only 90% done. That's why we have reviews like this, to catch when we skip over steps or missed something important, before it is too late. So, let's take a step back, take a breath, and actually discuss in greater detail whether this really is a diagnostic option, what the spectrum of exposure options is, etc. (But, not here. The next step is to go back to the development forum.) (This crossed with your suggestion to think on it more over the holidays and resume then; that is a good plan. Happy holidays everyone!)
17-12-2020
With that, I am (like many others) going back on vacation. Let the discussion continue, and while we should not be jumping haphazardly into accepting the Alternative 2, at the same time we should not, IMO, jumping haphazardly into backing out the change from JDK 16. The code in question does not introduce JCK test failures (or other test failures) I am currently aware of, the code paths it touches are pretty isolated, and thus it can be backed out without problem a bit later. Let's step away, take a breath, run the arguments at the back of our minds, and then we decide what to do after the Christmas cheer is over.
17-12-2020
Alternative 2 is implemented in JDK-8258619, JMH change is prototyped in CODETOOLS-7902813. Brian, while you are late at this party, others have already discussed that accepting only the empty methods is one of the answers. Vladimir [actually proposed][1] it to address Thomas' lingering compatibility questions. Thomas [said explicitly][2] that empty methods "would be perfect". This is why it was listed as the alternative in this CSR, not taken until you insisted the compatibility issue is still there. The only person who was resisting that alternative was myself, because it would have required more JMH changes. Your point about compatibility issue still does not ring true to me, but "third time a charm" -- I have now prototyped them, and compiler-assisted blackholes still work, and so my resistance is lifted. Sure, we can (and should) wait for any other comments on this, but this approach is not something unheard of before. It would seem three of us converged on this solution. The actionable question remains: do you (and whoever are the others that should) agree too? There is still a plenty of time to review this. Thing is, delaying the blackhole support means the _actually important compiler implementation part_ would not get tested experimentally "in the wild". There is no reason in my mind to expose the public API (or do any other API changes, for that matter), until we are sure that benchmark harnesses like JMH can utilize this feature without problems, or what those problems might be. This is why the lowest commitment possible -- diagnostic compiler command -- is a good fit here. That reasoning is in CSR text, and it seems to be accepted by JIT compiler folks. [1]: https://github.com/openjdk/jdk/pull/1203#issuecomment-739886504 [2]: https://github.com/openjdk/jdk/pull/1203#issuecomment-739888384
17-12-2020
> Brian, what is left to investigate for Alternative #2? Hmm, we seem to have jumped from "In a hypothetical world, assuming all the discussions have already played out, this might be a possible solution" to "all those discussions have happened, we've done all the investigations, and we've all agreed this *is* the answer." This is exactly the sort of short-circuiting of the process that got us into this trouble in the first place. The high-order bit here is: more discussion is required. Maybe, in the end, "it's OK for empty methods" is the right answer; discussion will disclose this. But many things are not obvious here, including what is the right approach for exposing this (public API, Unsafe, something else), whether (assuming the answer is "the same something else that was proposed") that restricting it to empty methods is suitable (and, if so, how we would define 'empty', and specifying what we do when the method is not empty), and whether it is even suitable to call this a "diagnostic" flag (this does not seem obvious to me yet.) All of these are nontrivial discussions, and we should not let the deadline bully us into short-circuiting them, just because a train is leaving soon. Which is why the right thing to do is to recognize we caught something nontrivial in CSR, be sad about it for a minute, and then continue the discussion.
17-12-2020
Brian, what is left to investigate for Alternative #2? I believe both Thomas W, Vladimir I and me agree that it keeps the observed functional behavior the same, and thus eliminates the alleged problem. The flag is still diagnostic, so prior agreement also stays in place. Do you agree this resolves the conformance issue *you* raised? I think the amendment -- which pretty much adds the empty method body check -- is pretty doable in RDP1 timeframe: RDP2 is Jan 14 (*two weeks* into the new year), so there is about a month wall clock time, out of which I am sure at least a week of work time for everybody (otherwise RDP1 -> RDP2 makes little sense with winter releases :/).
17-12-2020
[~shade] Speaking hypothetically, yes, it seems possible that Alternative #2 could be a way out of the conformance issue raised here, and is worth investigating further. (But I don't see how this could realistically happen quickly enough to be specified, considered, and reviewed before RDP2. We're all on holiday or about to be, and the deadline is only a week into the new year.)
17-12-2020
> But, while I understand why you don't want this to be a conformance > issue, there really is no question about it. This is asking the JIT to > change the semantics of a method based on a command line argument. Alas, this Volker's argument is the evidence that "no questions about it" is demonstrably not true :/ I am on vacation, so cannot reply thoroughly. But hypothetically speaking, would **Alternative 2**, as stated in CSR body, get this issue (and hopefully all subsequent) resolve whatever conformance issue there allegedly is?
17-12-2020
Hi Volker; Yes, exploring `Unsafe` as a possible home for this is also a possibility. But, while I understand why you don't want this to be a conformance issue, there really is no question about it. This is asking the JIT to change the semantics of a method based on a command line argument. I also understand why it is attractive to try and label this a "diagnostic" feature, but that is also pretty clearly an abuse "diagnostic". The JCK UG has a definition for what "diagnostic" means, and this is way outside of that. I agree that everyone here worked in good faith, and we got to the current state honestly, and it is no fun that we're discovering only at, what we thought was the end, that there are conformance concerns that were not identified earlier. It is unfortunate that the change has to be backed out, but that's the responsible move. We'll keep working at it, I'm sure we'll get there.
17-12-2020
I think Brian's post needs some further discusion and clarification. First of all, I'm not against implementing the BlackHole functionality as a public API with SE scope. On the other hand, BH's are a quite exotic feature for a very specific and limited audience and as such I really doubt if it justifies an exposure as an SE API. We have other places for such implementation-specific features namely `jdk.internal.misc.Unsafe` or the `WhiteBox` API. One of the problems with implementing BHs as a SE API is that there will be no way to downport such an arguably useful feature to older jdk releases. So maybe an intermediate step could be to first implement such a feature in an internal API like `Unsafe` and later, after it has proved useful, expose it as an SE API. This would also give other interested parties the possibility to use, evaluate and potentially improve the feature before it gets standardized. (I know we also have Incubator Modules and Preview features for this purpose, but they both can't be easily downported to older versions either). I disagree with Brian's statement that the current feature "*has a serious conformance problem*" and I think his conclusion that this change imposes a "*go directly to jail*" problem is pure scaremongering. First of all, please let me know if we all still agree to the [current status quo which unambiguously declares](https://wiki.openjdk.java.net/display/HotSpot/Hotspot+Command-line+Flags%3A+Kinds%2C+Lifecycle+and+the+CSR+Process) that "*Diagnostic flags are not meant for VM tuning or for product modes.*" and "*for CSR purposes, only Hotspot's 'product' and 'manageable' flags are considered exported interfaces that require a CSR request when they are added*"? Assuming this assumption still holds and without loss of generality, let me cite the parts from the [JCK 6b Users Guide](https://github.com/simonis/JCK6bUsersGuide/blob/master/JCK6b_Users_Guide.pdf) (unfortunately this is the only JCK version we can publicly speak about) which in my oponin are relevant to your "*go directly to jail*" proposition: > First some definitions: > > - *Documented*: Made technically accessible and made known to users, typically by means such as marketing materials, product documentation, usage messages, or developer support programs. > - *Operating Mode*: Any Documented option of a Product that can be changed by a user in order to modify the behaviour of the Product. > - *Product Configuration*: A specific setting or instantiation of an Operating Mode. > >And now to the rules: > > �� 2.2.2 Rules for Java SE 6 Technology Products: > > 1. The Product must be able to satisfy all applicable compatibility requirements, including passing all Conformance Tests, in every Product Configuration and in every combination of Product Configurations, except only as specifically exempted by these Rules. > > a. If an Operating Mode controls a Resource necessary for the basic execution of the Test Suite, testing may always use a Product Configuration of that Operating Mode providing that Resource, even if other Product Configurations do not provide that Resource. Notwithstanding such exceptions, each Product must have at least one set of Product Configurations of such Operating Modes that is able to pass all the Conformance Tests. First of all it is not completely clear what a *Product Configuration* really is. Oracle documents a bunch of the [extended -XX: command line options](https://docs.oracle.com/en/java/javase/11/tools/java.html), but by far not all of them. So what counts as *documented option*? Options on the list mentioned before, or the options you get when running `java -XX:+PrintFlagsFinal`, the options you get when running `java -XX:+UnlockDiagnosticVMOptions -XX:+PrintFlagsFinal` or maybe even the full blown list of options you get when running `java -XX:+UnlockDiagnosticVMOptions -XX:+UnlockExperimentalVMOptions -XX:+PrintFlagsFinal -version`? Assuming the whole OpenJDK community is acting in "good faith" and taking into account that ["Diagnostic flags are not meant...for product modes."](https://wiki.openjdk.java.net/display/HotSpot/Hotspot+Command-line+Flags%3A+Kinds%2C+Lifecycle+and+the+CSR+Process) I think it is a valid interpretation that diagnostic and experimental flags are not considered part of a *Product Configuration* which has "*to satisfy all applicable compatibility requirements*". And even if they would be considered part of a *Product Configuration*, ��2.2.2.a still leaves us the possibility that just "at least one set of Product Configurations of such Operating Modes...is able to pass all the Conformance Tests*" (`-XX:CompileCommand=` would be such a trivial Operating Mode). Finally, just to make this discussion a little less theoretical and boring I want to quickly mention the `-XX:AbortVMOnException` diagnostic option, which is supported in Open- and OracleJDK since many years and obviously "*modifies the semantics of exceptions*" if set. I sincerely do not belive that anybody who has done a Java SE certification in the last years would have to fear "*going directly to jail*" just because a *Product Configuration* with `-XX:AbortVMOnException=java.lang.IndexOutOfBoundsException` of the product he has certified will not "*satisfy all applicable compatibility requirements*". To cut a long story short and taking into account that we are already in RDP1, I suggest to let current feature in jdk16 as it is. After all, it is just a diagnostic option which can be easily removed or changed later, even without a CSR. If the feature will be used and proves useful, we can change it into an API with jdk or SE scope at any time later.
17-12-2020
Given Brian's comment above, I'm pending this request. As noted in a CSR FAQ (https://wiki.openjdk.java.net/display/csr/CSR+FAQs), if a retroactive CSR is done, the changes that can be needed afterward can include removal of the already-pushed change. I've filed bug JDK-8258558 for that purpose. Please file a different bug and CSR when pursuing the API-based solution Brian has suggested.
17-12-2020
My apologies for entering mid-performance, this CSR has only recently come to my attention. As far as I can tell, the history here runs a little backwards: - Much discussion and development iteration on the best way to implement better blackhole support in HotSpot - The code was pushed - The late realization that a CSR is needed - The still-later realization (upon discussion with JCK experts) that the (clever) approach of using the compile command has conformance issues. (This is one reason why we do CSR; to ensure that these issues are thought through.) First, let me say that I support this feature. I use JMH, I use JMH blackholes, and I support improving the implementation so that they impose less distortion on what is being measured. And I am satisfied with the implementation review done by the HotSpot experts; if they are satisfied that the implementation is sound, so am I. Unfortunately, there are serious issues with the way it is exposed, and these cannot be wished away. As a matter of "are we solving the right problem", I agree completely with the comment made [here](https://github.com/openjdk/jdk/pull/1203#issuecomment-739875574) by ThomasW that: > I think introducing a blackhole method in the JDK would be the right thing to do instead of short-cutting the process. In other words, if this feature is worth doing (I agree it is), it is worth doing right. I think we've fallen victim to the all-too-common "Doing it right would take too long, so let's put our spec-lawyer hat on and try to justify why we can do it an easier way." Which is a totally understandable temptation (and sometimes works). Critically, the feature, as defined, has a serious conformance problem; the semantics of a method are being modified by a command-line flag. That's a "go directly to jail" problem, and this changeset cannot go in as is. (Yes, I realize it is in already, but that's not really relevant.) The right thing to do here seems to be to expose an SE-scoped, do-nothing method, which can act, with the spec's blessing, as an intrinisification point. There is ample precedent for this; the `Thread::onSpinWait` method was introduced for a very similar reason, as was `Reference::reachabilityFence`. Implementations are permitted to do nothing special with these methods, but the spec permits implementations to do more, and guides users to when they might use it profitably. This is the sort of spec blessing you need here. Maintenance of the incremental spec surface is trivial, because it is not required to do anything; all the maintenance is in the implementation, which we would have to do anyway. (And it needs to be SE-scoped; doing this as a JDK method has the same problem as doing it with a command line flag.) Further, there's another reason why this is a better solution; it provides the BH goodies to everyone, not just JMH users. You came to your solution honestly; you started with a problem in JMH, and got to the point where it needed VM help to get farther, and then proceeded to solve the problem of "how can the VM help JMH make better black holes." All good, but: that's not the real requirement, that's the yak shave that got us here. Framing it in terms of a platform feature to enable a JMH feature introduces accidental constraints (it spills over past JMH, but in a "made for JMH" sort of way); a black hole feature in the platform would be more broadly -- and directly -- usable. While I understand the concern of the HotSpot folks that this does not really qualify for "experimental" treatment, it also doesn't qualify for "diagnostic" treatment either. This isn't diagnostics. So, how to proceed? The main new task is writing and reviewing the specification for the new method (and, of course bikeshedding the name!); given that, intrinsifying the implementation in HotSpot seems easy enough. I don't think this is very hard, nor would I have any problem endorsing the CSR for it, though this sort of spec writing is always tedious and requires more rounds of review than we'd initially think. But I think we have to do it. Something like the following is a starting point: /** * Does nothing. This method is provided solely to facilitate accurate performance measurement in benchmarks, which are particularly susceptible to being biased by dead code elimination. Calling this method indicates that the argument is the result of a calculation that the user wishes to retain. Implementations are encouraged, but not required, to inline away this method, but treat the argument as being still reachable for purposes of liveness analysis. [ more detail ] @apiNote [ description of the problem, example of how to use the method ] @implNote [ description of what the reference implementation does, if desired ] */ Minimal would probably be versions for ref and int (or long); you can decide whether the other seven are important enough. Unfortunately, I'm not sure the calendar is with us on this; we're already in RDP1, holidays are upon us, and I think we'd want to spend a little time thinking about exactly the right way to word this. So the obvious remedy is to back out the integration, continue to work on the proper specification, and shoot for integrating in 17. But, we ship every six months, so a small delay should not be too painful. I realize this is disappointing, but this is worth getting right, and it shouldn't be too much more work to get there.
16-12-2020
JDK-8257848 is now integrated. This CSR can be closed, as it now relates to the change to "diagnostic" JVM subflag.
08-12-2020
I agree with the proposal as implemented in JDK-8257848. This needs to be a diagnostic command.
07-12-2020
I PR'ed JDK-8257848, mentioned it in CSR text. As far as I understand, once JDK-8257848 lands, this CSR becomes irrelevant. I will leave it open meanwhile.
07-12-2020
Sure! I did the patch that demotes the `blackhole` command to diagnostic option. I'll PR it and then update the CSR to mention it, and then let it be closed.
07-12-2020
My example with not_product flag may not be applied directly for this case. You would need to add new code into `compilerOracle` to check if `-XX:+UnlockDiagnosticVMOptions` is specified when such options are used. But then you can do the same for your format `-XX:CompileCommand=blackhole,<method>`. It is up to you. My main point is you don't need CSR if you don't change type of `CompileCommand` main flag and only add new diagnostic option to it.
07-12-2020
I said `diagnostic` because it makes more sense to me than `experimental` in this case. `Diagnostic` flags are also not require CSR: https://wiki.openjdk.java.net/display/HotSpot/Hotspot+Command-line+Flags%3A+Kinds%2C+Lifecycle+and+the+CSR+Process
07-12-2020
I prefer **Alternative 1**. And to do that you don't need CSR, I think. You need to change format of the flag to `-XX:CompileCommand=option,<method>,blackhole ` and declare `blackhole` as diagnostic. We have already example for NOT_PRODUCT options: https://github.com/openjdk/jdk/blob/master/src/hotspot/share/compiler/compilerOracle.hpp#L91
07-12-2020
Looks good to me. - Regarding "Alternative 1" I would be fine to leave the current implementations "as is". However, should others mandate a change here, I'd suggest to turn the new option into a `Diagnostic` rather than an `Experimental` option because benchmarking for me is more of a diagnostic than an experimental task. - Regarding "Alternative 3" I'm also fine with the current state. Again, if others mandate a change here, you might consider changing intrinsification of a JMH method into the intrinsification of a new method in the WhiteBox API instead and call that one from your Blackhole implementation (I understand that such an implementation would require changes and support for different versions in JMH as mentioned in "Alternative 2").
07-12-2020