Re: RFR: 8341028: Do not use lambdas or method refs for verifyConstantPool [v3]
On Mon, 21 Oct 2024 14:13:35 GMT, David M. Lloyd wrote: >> Currently, `ParserVerifier#verifyConstantPool` uses a `switch` to produce a >> `Runnable`, which is consumed by a `Consumer` (instantiated within >> a loop) which runs the task inside if a `try`/`catch`. We can eliminate a >> number of lambdas and method references, plus some allocation pressure, in >> this code by simplifying it so that the `switch` is itself run directly >> within the `try`/`catch`. > > David M. Lloyd has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains two commits: > > - Make sure that we record every error instead of stopping at the first > error in a particular CPE > - 8341028: Do not use lambdas or method refs for verifyConstantPool > >Currently, `ParserVerifier#verifyConstantPool` uses a `switch` to produce > a `Runnable`, which is consumed by a `Consumer` (instantiated > within a loop) which runs the task inside if a `try`/`catch`. We can > eliminate a number of lambdas and method references, plus some allocation > pressure, in this code by simplifying it so that the `switch` is itself run > directly within the `try`/`catch`. Latest version looks good. @liach Do you need to rereview? - Marked as reviewed by adinn (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/21209#pullrequestreview-2399769060 PR Comment: https://git.openjdk.org/jdk/pull/21209#issuecomment-244693
Re: RFR: 8293336: AOT-linking of invokedynamic for lambda expression and string concat
On Mon, 23 Sep 2024 18:45:49 GMT, Ioi Lam wrote: > This is the 7th and final PR for [JEP 483: Ahead-of-Time Class Loading & > Linking](https://bugs.openjdk.org/browse/JDK-8315737). > > This PR implements the AOT-linking of invokedynamic callsites: > - We only link lambda expressions (`LambdaMetafactory::metafactory`) and > string concat (`StringConcatFactory::makeConcatWithConstants()`) as the > results of these bootstrap methods do not have environment dependencies and > can be safely cached. > - The resolved CallSites are represented as Java heap objects. Thus, this > optimizations is supported only for the static CDS archive, which can store > heap objects. The dynamic CDS archive is not supported. > > **Review Notes:** > > - Start with `AOTConstantPoolResolver::preresolve_indy_cp_entries()` -- it > checks all indys that were linked during the training run, and aot-links only > those for lambdas and string concats > - `SystemDictionaryShared::find_all_archivable_classes()` finds all the > hidden classes that are reachable from the indy CallSites > - In `MetaspaceShared::preload_and_dump_impl()` we call > `MethodType::createArchivedObjects()` to record all MethodTypes that were > created in the assembly phase. This is necessary because the identity of > MethodTypes is significant (they are compared with the `==` operator). Also > see MethodType.java for the corresponding code that are used in the > production run. > - Afterwards, we enter the safepoint (`VM_PopulateDumpSharedSpace::doit()`). > In this safepoint, > `ConstantPoolCache::remove_resolved_indy_entries_if_non_deterministic()` is > called to remove any resolved indy callsites that cannot be archived. (Such > CallSites may be created as a side effect of Java code execution in the > assembly phase. E.g., the bootstrap of the module system). > > **Rough Edges:** > > - Many archived CallSites references (directly or indirectly) the static > fields of the classes listed under > `AOTClassInitializer::can_archive_initialized_mirror()`, where the object > identity of these static fields is significant. Therefore, we must preserve > the initialized states of these classes. Otherwise, we might run into > problems such as [JDK-8340836](https://bugs.openjdk.org/browse/JDK-8340836). > Unfortunately, the list is created by trial-and-error, and may need to be > updated to match changes in the `java.lang.invoke` and > `jdk.internal.constant` packages. We expect Project Leyden to come with a > general solution to this problem. > - If the identity is significant for a static field in a complex class, but > not all of the static fields of this cl... src/hotspot/share/cds/aotConstantPoolResolver.cpp line 450: > 448: > 449: Symbol* mh_sig = cp->method_handle_signature_ref_at(mh_index); > 450: if (!check_lambda_metafactory_signature(cp, mh_sig, false)) { This appears to be rerunning the previous check without a log message -- or am I missing something here? src/hotspot/share/cds/archiveBuilder.cpp line 234: > 232: _klasses->append(klass); > 233: if (klass->is_hidden() && klass->is_instance_klass()) { > 234: update_hidden_class_loader_type(InstanceKlass::cast(klass)); Can you explain why this 'update' is needed. Are the shared classloader type and classpath index not already set? Or do they need adjusting? Perhaps a comment would help. src/hotspot/share/cds/cdsConfig.cpp line 486: > 484: bool CDSConfig::allow_only_single_java_thread() { > 485: // See comments in JVM_StartThread() > 486: return is_dumping_static_archive(); The test in `JVM_StartThread()` calls `CDSConfig::is_dumping_static_archive()`. Should it be updated to call `CDSConfig::allow_only_single_java_thread()`? src/hotspot/share/cds/cdsHeapVerifier.cpp line 126: > 124: > 125: // Integer for 0 and 1 are in java/lang/Integer$IntegerCache and are > archived > 126: ADD_EXCL("sun/invoke/util/ValueConversions", "ONE_INT", > // E At line 46 the example that explains how the verifier works discusses save and restore of field `Foo#archivedFoo`. I believe that field was supposed to be declared as `static` but it is actually declared as an instance field. src/hotspot/share/cds/cdsHeapVerifier.cpp line 274: > 272: char* class_name = info->_holder->name()->as_C_string(); > 273: char* field_name = info->_name->as_C_string(); > 274: if (strstr(class_name, > "java/lang/invoke/BoundMethodHandle$Species_") == class_name && Can we have a comment to explain this special case? src/hotspot/share/cds/classListParser.cpp line 609: > 607: // We store a pre-generated version of the lambda proxy class in the > AOT cache, > 608: // which will be loaded via JVM_LookupLambdaProxyClassFromArchive(). > 609: // This eliminate dynamic class generation of the proxy class, but we > still need to "This eliminate" --> "This eliminates" src/hotspot/share/cds/heapShared.cpp line 893: > 891: void KlassS
Re: [External] : Re: New candidate JEP: 471: Deprecate the Memory-Access Methods in sun.misc.Unsafe for Removal
On 13/06/2024 14:39, Alan Bateman wrote: Good to hear you've got a prototype to discuss. I don't think I can look at what you have in your own repo but I do have a question. Do the defaultXXX methods return a method handle or do they fail/null when there are read/writeObject methods? Asking if they will bypass these methods. Why can you not look at David's repo? He is a JDK author (dmlloyd) and this work is being done as part of his employment by Red Hat. regards, Andrew Dinn ---
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v24]
On Fri, 5 Apr 2024 09:31:18 GMT, Severin Gehwolf wrote: >> Kind of aligning with the "Donaudampfschiffahrtsgesellschaftskapitän" >> prejudice of German. ;-) >> >> >> >> (In Sweden, we have "flaggstångsknoppsförgyllare" so you are not alone) > > Hah! My kids just recently informed me about > "Donaudampfschiffahrtsgesellschaftskapitänsmützenproductionsstätte"... or > whatever else you can come up with :) Hmm, that's nothing. Look up Rhabarberbarbara on YouTube. - PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1553444805
Re: RFR: 8200559: Java agents doing instrumentation need a means to define auxiliary classes [v2]
On Sat, 27 Jan 2024 05:11:28 GMT, Alexander Kriegisch wrote: > Bytecode transformation should not be rocket science, but it progressively is > developing in that direction. Hmm? Bytecode transformation of the JDK runtime implementation is a lot more complicated than your comments seem to acknowledge and, here's the important thing, *it always has been*. You need to remember that instrumenting JDK runtime code involves rebuilding the engine that you are driving while you are in mid-flight. If you think there are few-to-none hidden gotchas involved in doing that then I suggest you are significantly underestimating the opportunity for things to go wrong -- not just when it comes to instrumenting some specific release of OpenJDK but also when it comes to keeping instrumentation working across legacy and future releases, with all the variety of moving parts that the (necessary) development of the platform requires. The same observation explains why project Jigsaw was needed. The danger of clients using internal JDK runtime APIs -- especially the core runtime APIs -- is much more subtle than many of the programmers who have routinely relied on it recognize. The biggest threat is that public runtime APIs are often implemented via calls to multiple internal APIs -- which may themselves involve multiple entries and re-entries to the JVM. It has always been (and always will be) the case that an isolated call from a client to an internal API can leave the JDK runtime and/or the JVM in an incoherent state because correct use of that internal API requires a correct sequence of invocations with matched inputs and outputs. It is easy even for OpenJDK developers to fails to get this right, especially when calls involve entry to the JVM. The possibility for a programmer who is not very familiar with the JDK runtime code and the JVM code to get it wrong are significant. Worse, the problems may not manifest immediately or in all cases so the danger can be unapparent until disaster strikes. - PR Comment: https://git.openjdk.org/jdk/pull/3546#issuecomment-1914812297
Re: RFR: 8200559: Java agents doing instrumentation need a means to define auxiliary classes [v2]
On Sun, 28 Jan 2024 22:33:01 GMT, Rafael Winterhalter wrote: > What stops people from supplying a fake instance? Wouldn't you need to "test > run" the instance first? Not necessarily. When the generated API implementation relies on the capabilities of class `Instrumentation` -- such as opening modules -- to implement the invoked operation the obvious answer is that a fake instance just won't work. However, if you want the implementation to validate an incoming call you can easily arrange for that. For example, provide a method on the agent class that says yes to its own instance and no for any other instances e.g. class AgentClass { private static Instrumentation myInst = null; void premain(Instrumentation inst) { myInst = inst; . . . } static boolean validate(Instrumentation inst) { return myInst != null && inst == myInst; } . . . } Method validate can be used to ensure API calls only proceed when invoked by the agent or code that the agent trusts. - PR Comment: https://git.openjdk.org/jdk/pull/3546#issuecomment-1914771074
Re: RFR: 8200559: Java agents doing instrumentation need a means to define auxiliary classes [v2]
On Fri, 26 Jan 2024 10:17:16 GMT, Alexander Kriegisch wrote: >>> @AlanBateman, the AspectJ weaving agent creates an auxiliary class to >>> implement an "around" advice for a method, i.e. a method execution is >>> intercepted and the user has options to do something before optionally >>> calling the target method and then do something afterwards too. In doing >>> so, she can modify method arguments before calling the target method, then >>> also modify the result. Instead of calling the target method, she may also >>> return a result of the expected type instead. Before, after or instead of >>> calling the target method, she can also throw an exception. >>> >>> The target class is transformed in such a way to call the auxiliary class, >>> which necessitates the the aux-class to be in the same classloader as the >>> target class. But because the aux-class is defined while the target class >>> is still being transformed during class-loading, we cannot have any look-up >>> instance pointinmg to it yet. >> >> Right, this is what JDK-8200559 was originally about. Mandy and I discussed >> it several times and load-time instrumentation that defines auxiliary >> classes in the same run-time package is a reasonable addition. >> >> The more general request for an "unrestricted defineClass" conflicts with >> several ongoing efforts so we've had to kick that to touch. > >> load-time instrumentation that defines auxiliary classes in the same >> run-time package is a reasonable addition > > Thanks for finding some common ground. I appreciate it. > >> The more general request for an "unrestricted defineClass" conflicts with >> several ongoing efforts so we've had to kick that to touch. > > I better do not start to share my general opinion about JMS _(edit: sorry, > couldn't resist in the end)_ - I mean the ongoing effort that started with > Jigsaw - in detail, because that would end up in a flame war. > > Let me just say, that "well-meant" is not the necessarily same as > "constructive", "helpful" or "necessary". Back when Jigsaw was designed, it > seemed to be a good idea. But it has been introduced a long time ago, and all > my enterprise customers since then are trying to ignore its existence as much > as they can, mostly seeing it as an impediment or at least a major annoyance. > I think, one of the reasons why Python gained so much traction for > enterprise-level big data and machine learning stuff is that you have a > dynamic language environment in which you can pretty much do whatever you > want and just get things done. > > Those same enterprise customers want to use the tools of their choosing with > as many and as easy to use features as possible. They do not trade those > features for security, but implement security in a different way, such as > devising a micro service architecture and isolating containers from each > other in OpenShift or what have you, defining service mesh rules on top of > that, if necessary. > > Deprecating security managers was a laudable idea, but adding those > unnecessary restrictions in the JVM in exchange was kind of one step forward, > two steps back. They just get in the way of getting useful things done. This > is what a language and a VM are for: getting things done in a productive way > with as few barriers as possible. Instead, with every new Java release, users > and tool providers alike are forced to jump through more hoops. There is so > much great, innovative stuff in the JVM and Java SE API. E.g., virtual > threads are the greatest thing since bread came sliced. We want more of that > and fewer barriers. Streams, NIO, the whole lambda and functional bunch of > stuff, structured concurrency, better GC, records and neat ways to > deconstruct them etc. - wow, great stuff. But new "security features" (a.k.a. > restrictions) like modules and sealed classes - not so great from a > productivity perspective. Since JDK 9, I have seen generations of developer... @kriegaex Luckily, you and your customers are not obliged to use the JPMS, nor find it useful for whatever libraries or apps you write or deploy. However, the fact that you or many other programmers do not use it does not mean it has not been a success. Anyone deeply involved with JDK and/or JVM development in recent years knows that it has been and continues to be critical to maintaining and extending the Java platform. Regarding my previous comment about Byteman using its own dedicated, dynamic module to provide secure access to MethodLookup instance you might want to look at the relevant code. It relies on a sanctioned API of Instrumentation that was introduced as part of the negotiation of JPMS integration precisely to allow agents to interact with and reconfigurethe module system at runtime. The resulting Byteman code provides a simple API that allows methods to be executed indirectly, either via reflection in jdk8- or via method handles in jdk9+. Yo
Re: RFR: 8200559: Java agents doing instrumentation need a means to define auxiliary classes [v2]
On Thu, 25 Jan 2024 12:16:13 GMT, Rafael Winterhalter wrote: > Requiring such an API opens the module to anybody, though, punching a hole > into the module boundary. How so? Any module created to print Lookups can easily rely on a shared secret to secure the API. Byteman employs a non-null Instrumentation object (a value which any agent ought to keep secret). However, it could just as easily have employed an arbitrary bit length hash key. The key can be used to initialize a module-private static long[] field of an API implementation class generated into the module i.e. the hole can actually be a keyhole in the shape of a key known only to the API client and implementation. - PR Comment: https://git.openjdk.org/jdk/pull/3546#issuecomment-1910230873
Re: RFR: 8200559: Java agents doing instrumentation need a means to define auxiliary classes [v2]
On Thu, 25 Jan 2024 06:39:56 GMT, Alexander Kriegisch wrote: >> Setting '-javaagent' is mainly an operations problem. Many build tools do >> not allow to declare a test dependency this way as the life cycles are not >> laid out for it, the internal repository location might be machine >> dependent, for example, and it's difficult to point to a specific folder >> and file reliably. In this case, I'd say it would be easier to specify a >> parameter in the sense of '-Djdk.attach.allowAttachSelf=true' as it is a >> static value. This would however only work for build tools that initiate a >> new VM for running tests which is not overly attractive for simple builds >> due to the overhead. Of course, Maven, Gradle and similar tools could set >> this parameter by default for their own JVM, that part is likely >> overcomeable but it will certainly create confusion about how to run >> libraries that are omnipresent today and make the JVM ecosystem less >> approachable. >> >> What bothers me more is the tooling perspective for non-self-attached >> agents. For example, Aaeron offers a Java agent that adds plenty of debug >> logging to relevant lines of code. This affects method size and so forth, >> with Aaeron as a high-performance tool for banking and finance which is >> written very consciously with regards to the JIT, adding it directly was >> not feasible. Normally this logging is therefore thrown into a VM in >> retrospect, once an application starts failing and is already taken off the >> load balancer. For such a post-mortem, it would be rather annoying to >> realize that a JVM cannot be attached to with full capabilities if you >> forgot to set some flag. And often you did of course not consider the VM >> instance to fail, sometimes it takes months to get a JVM into this buggy >> state. This would be fairly inconvenient to face. >> >> Therefore, I really hope that the dynamic attach from 'outside' the VM will >> survive without imposing limits and that rather the self-attachment problem >> will be targeted as such. When I mention a 'jdk.test' module in the Mockito >> context, I am also rather hoping to improve performance compared to >> convenience. The problem with '-Djdk.attach.allowAttachSelf=true' is that >> you still need to start a new VM etc. Since Java 9, running single tests >> with Mockito has for example become much slower compared to Java 8. Despite >> the startup performance improvements in the JVM. If one could avoid the >> location-bound '-javaagent:...', but get the Instrumentation instance >> directly, I think this would render a real ... > > @raphw and everyone else, I apologise for commenting on an auto-closed issue, > but I have no other way of contacting anyone, because neither do I have an > account to comment on > [JDK-8200559](https://bugs.openjdk.org/browse/JDK-8200559) nor am I a member > in any other of the JDK development or "JVM rock star" circles for good > reason: I simply have a very limited understanding of the topic at hand. > > I am, however, the current maintainer and project lead of Eclipse AspectJ, > and since JDK 16 the weaving agent requires `--add-opens > java.base/java.lang=ALL-UNNAMED` along with > `-javaagent:/path/to/aspectjweaver.jar` for the reason my predecessor Andy > Clement has explained in [AspectJ bug > 546305](https://bugs.eclipse.org/bugs/show_bug.cgi?id=546305), which is also > linked to JDK-8200559. @mlchung was also involved in the discussion there. > > Here, there has been a lot of discussion about Byte Buddy and Mockito. For > AspectJ, which is widely used in the industry, not just in Spring but also in > countless other productive contexts, there still is no solution to the > problem that we need to instrument a target class to call an auxiliary class > (generated by the agent) that does not have a Lookup object on the target > class to begin with. The target class is still being transformed, i.e. not > loaded yet, and hence we cannot look it up. > > Is this case being considered? Can any of you advise me how to solve that > problem? I am open for advice and hands-on chat, desktop sharing or > audio/video call sessions. @kriegaex I resolved this problem in Byteman by opening up the module of the target class to a module (dynamically) created by Byteman. The Instrumentation instance provided to the agent allows you to perform such an opens operation at runtime rather than on the command line. Byteman relies on an API provided by this module to create the lookups and hand them back for use in woven code. n.b. although this relies on the module exporting a public API, that API can be secured by requiring callers to pass a non-null Instrumentation instance i.e. to have agent capabilities. - PR Comment: https://git.openjdk.org/jdk/pull/3546#issuecomment-1909801331
Re: RFR: 8309191: Reduce JDK dependencies of cgroup support
On Tue, 13 Jun 2023 15:48:31 GMT, Christian Wimmer wrote: > We (as the Native Image team) are OK with this. Our testing will detect that > pretty quickly, and then the new code can be fixed. That may well be the case. However, until all the concerns raised by OpenJDK reviewers who have looked at this PR are addressed to their satisfaction it would not be appropriate to merge this patch. n.b. That does not automatically mean the course of action the reviewers have recommended has to be followed. A resolution needs to be negotiated according to the merits and risks of the change. However, regarding that negotiation, I'll observe that the (repeated) request to break this change down in several steps appears to me to be motivated by the desire to ensure that the merits of the change are maximized (no unnecessary loss of important functionality) and the risks minimized (no unnecessary perturbation of the current implementation) -- which is not an unusual way for OpenJDK reviewers to proceed. - PR Comment: https://git.openjdk.org/jdk/pull/14216#issuecomment-1598400830