<div dir="ltr"><div>Hello again,<br></div><div><br></div><div>Thanks for merging my patches, the class reader API works more or less equivalent to the ASM version. The one thing I have not working so far are attributes that the JDK knows but ASM does not. I was trying to implement the "contents" method upwards, but this is difficult with the unbound attributes as they normally require a BufWriter which in turn needs a ConstantPoolBuilder. Somehow, I need to pipe the constant resolution to ASM which cannot be done without unsealing some interfaces. I will try to prototype a solution here, too, but I wanted to get the writer working first.</div><div><br></div><div>With the writer, I have made some progress after adding a monadic view to ClassBuilder where one can apply a consumer multiple times before "closing" the writer for extracting a class file. I pushed this experiment on a commit of my clone (<a href="https://github.com/raphw/jdk-sandbox/commit/2be58f400b9ebf96b851eda658e0b8d2560421c5">https://github.com/raphw/jdk-sandbox/commit/2be58f400b9ebf96b851eda658e0b8d2560421c5</a>) to showcase the way I thought this might work. In theory, it should allow for any optimization of the current API. At the same time, it is awkward enough that people would only use it if they really needed it and therefore avoid it by default. And once they use it, any IDE would ask for closing each intermediate object when detecting the AutoCloseable interface. The only thing that I had to compromise on compared to "non-open" API was the use of CodeBuilderImpl which is currently reapplying the consumer in case of a LabelOverflowException. At the same time, I hoped that this might be a temporary state anyways as the possible reapplication is unlikely to be expected by any user.</div><div><br></div><div>For the type annotations on instructions: Would it be an option to add "getVisibleAnnotations" and "getInvisibleAnnotations" to the relevant CodeElement types? This way, I could for example query the "ExceptionCatch" value for its annotations. <br></div><div><br></div><div>StackMapFrames could on the other hand just be added at their position in the CodeElement iteration to receive them where they become relevant. This way, one would not need to keep track of the current offset. This would also allow for an easier write model where ASM does not allow you to know the offset of a stack map. I assume that the current model is very much modeled after the needs of the javap tool. Ideally, the frame objects would be reduced to the information that is contained in a class file and the consumer could track implicit information such as the "effective" stack and locals.</div><div><br></div><div>Java agents do not normally compute stack map frames as it requires to determine the most specific common supertype. This is not always possible when classes are not yet loaded as the type information is not yet available. Byte Buddy, for example, uses Java templates where two javac-compiled classes are merged and the stack map frames of both classes are combined on the fly. This works with a single pass and does not require a recomputation what makes it also very efficient. This is why I believe there should be an explicit way of defining stack map frames.<br></div><div><br></div><div>If you wanted to see my attempt on a writer using my suggested "monad" API for writers, here's my status quo: <a href="https://github.com/raphw/asm-jdk-bridge/compare/main...writer-poc">https://github.com/raphw/asm-jdk-bridge/compare/main...writer-poc</a></div><div><br></div><div>At last, some notes I made while working with the writer API:</div><div>- ClassBuilder does not require flags when creating a field using "withField", but does so for "withMethod". Should this not be consistent?</div><div>- EnclosingMethodAttribute requires optional parameters in its "of" method. I find that a bit awkward and would rather see an overload or the parameters to accept "null".</div><div><br></div><div>Thanks, Rafael<br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">Am Fr., 1. Juli 2022 um 17:02 Uhr schrieb Brian Goetz <<a href="mailto:brian.goetz@oracle.com">brian.goetz@oracle.com</a>>:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Thanks for test-driving this!<br>
<br>
> This works really well, congratulations to such a well-working first <br>
> attempt. Some details stuck out however:<br>
<br>
Good to hear.<br>
<br>
> 1. It does not seem like it is possible to model “CROP” frames. The <br>
> frames that are read include zero additional local variables, but they <br>
> do not indicate how many local variables are cropped. I would suggest <br>
> adding subinterfaces for all frame types to allow for the same pattern <br>
> matching style that works well with the rest of the API. This way, the <br>
> declaredLocals and declaredStack methods would only be available if <br>
> relevant and the crop frame type could add a croppedLocals() : int method.<br>
<br>
In general, the API for stack maps hasn't gotten a lot of attention, as <br>
there is not a lot of user code that accesses stack maps at all, and we <br>
generate stack maps as part of code generation.  So I'm not surprised <br>
there are missing bits or rough edges (same for next issue.)  Are you <br>
generating stack maps yourself?  Is the generator in the library not <br>
meeting your needs?<br>
<br>
> 3. Debugging with toString works well for the most, but not all <br>
> classes, for example subclasses of CodeElement have representations. <br>
> It’s probably an oversight but it would be neat to add this quickly to <br>
> make exploring the code easier.<br>
<br>
We should make a pass through all the implementations and make sure that <br>
toString is both present and uniform.  I am not surprised there are gaps <br>
here.  That would be a fine "starter bug" for someone to grab.<br>
<br>
> 4. The JDK code knows an attribute named CharacterRange. I must <br>
> confess that I never heard of it and I could neither find <br>
> documentation. I assume it is a JDK20 concept? This made me however <br>
> think about how such “unknown” attributes can be handled. I would like <br>
> to find a way to treat all attributes that I do not know or care about <br>
> as an UnknownAttribute. This way, I could simply forward them as a <br>
> binary dump, as I currently do it for custom attributes, for example <br>
> to forward it to ASM. However, today there is no way to convert an <br>
> attribute payload back to an array.<br>
<br>
It is not a JDK20 concept; it is what is used by the `jcov` coverage <br>
tool (and I believe others such as jacoco), from the CodeTools project.  <br>
Javac will generate this attribute if you ask it nicely.<br>
<br>
You should just ignore attributes you don't understand; that's how <br>
attributes were designed to work (there will always be new ones.) But <br>
your point about getting the raw contents is well taken. Essentially, <br>
your request is to raise `UnknownAttribute::contents` up to Attribute, <br>
perhaps renaming to `rawContents`.  This seems an easy and reasonable <br>
addition.<br>
<br>
> 5. I think the “findAttribute” method will be invoked a lot. <br>
> Currently, it iterates over all attributes on each call. Ideally, this <br>
> would only be done once to create a map of attributes with O(1) lookup <br>
> speed. Of course, I could do this myself, but I think this would be <br>
> such a common need that I wanted to suggest it as implementation for <br>
> the official API, especially since the API feels like a map lookup. <br>
> This could be done as easily as by storing attributes in a map after <br>
> they are read for the first time as the attribute keys already are <br>
> compared by identity.<br>
<br>
We considered this, and early performance tests suggested this was a <br>
loser.  However, that was a lot of API and workload churn ago, before <br>
AttributeMapper, which is interned, so we could try with an <br>
IdentityHashMap here and see whether it perturbs the benchmarks.<br>
<br>
> 6. I found that TypeAnnotation.CatchTarget offers an index of the <br>
> exception table entry, additionally to being visited inline. I found <br>
> that model a bit awkward as there is no indication of the index in the <br>
> ExceptionCatch instruction that comes with the same pass. Also, there <br>
> is no guarantee on when the type annotations are visited in relation <br>
> to the try catch block. Ideally, it would be guaranteed that the <br>
> annotation is visited directly after the ExceptionCatch pseudo <br>
> instruction, to allow for an easy single-pass processing.<br>
<br>
Yeah, everything about the type annotations attributes is a <br>
disaster...   Transforming while keeping the validity of the TA <br>
attributes is essentially impossible, because it includes indexes into <br>
things like exception tables, code attributes, tvar lists, tvar bounds <br>
lists, etc etc.  We can decode a TA attribute, we can give you a way to <br>
write one, but for a while we seriously considered just _dropping TA <br>
attributes on the floor during adaptation_ because they're so inherently <br>
unstable.<br>
<br>
The visitation order guarantee you ask for is pretty expensive, and <br>
something that very very few people would care about.  So I surely <br>
wouldn't want to do that by default.<br>
<br>
We've largely punted on doing anything better with type annotations, at <br>
least up until now, because they're such a disaster.  Still, we can <br>
probably do better here.  Open to suggestions.<br>
<br>
> Finally, I have not yet started working on a ClassWriter equivalent. <br>
> Here, I found that the style of the Consumer<ClassBuilder> to be <br>
> incompatible with the way ASM works. This is of course a decision of <br>
> style, but I would consider this a major difficulty to migrate current <br>
> code at some point. Would it be an idea to offer both styles of class <br>
> creation? The internal one, additionally to exposing an interface with <br>
> the methods of DirectClassBuilder?<br>
<br>
We've struggled with this as well when adapting the back end of <br>
`javac`.  This is a tricky issue.  In theory, allowing users to ask for <br>
a ClassBuilder is not hard, as you've sketched out.  But I am really <br>
worried it will become an attractive nuisance, where users will <br>
instinctively reach for that style because its familiar and seems <br>
"easier", and then will lose out on some features that the <br>
Consumer<ClassBuilder> approach offers, such as managing branch offset <br>
sizes.  Success would be if this were allowed, but only used 1% of the <br>
time.  Failure would be 3% :)<br>
<br>
Q: in this case, is it enough if only ClassBuilder has this option, or <br>
do you need it for MethodBuilder and CodeBuilder as well?<br>
</blockquote></div>