Re: RFR: 8341028: Do not use lambdas or method refs for verifyConstantPool [v3]

2024-10-28 Thread Andrew Dinn
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

2024-10-08 Thread Andrew Dinn
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

2024-06-17 Thread Andrew Dinn

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]

2024-04-05 Thread Andrew Dinn
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]

2024-01-29 Thread Andrew Dinn
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]

2024-01-29 Thread Andrew Dinn
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]

2024-01-26 Thread Andrew Dinn
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]

2024-01-25 Thread Andrew Dinn
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]

2024-01-25 Thread Andrew Dinn
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

2023-06-20 Thread Andrew Dinn
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