<div dir="ltr">Thanks for the feedback, Brian.<div><br></div><div>I've worked through the suggestions on the various ways to update an Attribute {use a stateful ClassTransform, use builder.original().get()} and found the "original().get()" approach felt the cleanest and clearest for what I was doing.</div><div><br></div><div>While working on that, I also added the ::with & ::withSymbols methods I had proposed and opened a PR [0] to see if the fleshed out API fit with the general way you wanted to evolve this. If that approach looks good, then I can extend it to the other attributes that may benefit from similar methods. (See the PR for a list)</div><div><br></div><div>The code using the new methods looks like:</div><div><br></div><div> ClassTransform.endHandler(b -> {<br> NestMembersAttribute newAttribute =<br> b.original().get().findAttribute(Attributes.NEST_MEMBERS)<br> .map(nma -> NestMembersAttribute.withSymbols(nma, nestMembers))</div><div> .orElse(NestMembersAttribute.ofSymbols(nestMembers));</div><div> b.with(newAttribute);<br> }<br></div><div><br></div><div>which ends up being a pretty straightforward way to split between creating a new attribute for just the new items creating a new attribute from the combo of old + additions.</div><div><br></div><div>--Dan</div><div><br></div><div>[0] <a href="https://github.com/openjdk/jdk-sandbox/pull/31">https://github.com/openjdk/jdk-sandbox/pull/31</a></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Aug 4, 2022 at 5:08 PM Brian Goetz <<a href="mailto:brian.goetz@oracle.com">brian.goetz@oracle.com</a>> wrote:<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 the experience report! Some comments inline.<br>
<br>
On 8/4/2022 4:19 PM, Dan Heidinga wrote:<br>
> Congrats to all involved in building this api! It's been easy to pick<br>
> up and get started with.<br>
<br>
Just as streams was the first API we "designed for lambdas", this is the <br>
first API we "designed for pattern matching."<br>
<br>
> The examples in the package-summary give a good starting point, though<br>
> I stumbled slightly when switching from purely reading to actually<br>
> transforming classfiles. I hit the "Writing classes" examples first<br>
> and rat-holed a bit there before realizing that I only needed the<br>
> ClassModel::transform call. It might be worth re-ordering the flow to<br>
> put transformation first as in my experience that's more common than<br>
> writing new classfiles.<br>
<br>
I think we also need a lot more examples about transformation. The <br>
transformation API may be simple, but there is some subtlety to it and <br>
it needs a lot more examples.<br>
<br>
> One of the areas I was modifying was the NEST_HOST and NEST_MEMBERS<br>
> attributes. Adapting an attribute, like adding new members to a<br>
> NEST_MEMBERS attribute, feels unpolished as it requires removing the<br>
> existing attribute and then adding an entirely new one. It would feel<br>
> more straightforward if there was a way to generate a new attribute<br>
> from the old with additions. Some kind of ::withSymbols method maybe?<br>
<br>
This use case (and similar: adding an annotation, adding an interface, <br>
etc) was something we struggled with for a long time. I rewrote the <br>
transformation API several times, and each time this case was on my <br>
mind. I am hoping that what is missing here is just the example in the <br>
documentation, rather than an entire new mechanism.<br>
<br>
The basic problem is that for any of the attributes that carry "list of <br>
stuff", and you want to add an item, you don't know until the end of the <br>
iteration whether you need to create a new attribute with one thing or <br>
transform the existing attribute. There are a few ways to do it, some <br>
require more hoop jumping than others.<br>
<br>
Note that the Transform interfaces, while SAM types, have some extra <br>
methods -- atStart(b) and atEnd(b). These are default methods whose <br>
default implementation does nothing, so that you can have a transform <br>
lambda, but if you want to use either of these methods in your <br>
transform, you need a class (anonymous or otherwise.)<br>
<br>
You also need to manage at least one bit of state: is there an element <br>
that carries the list of stuff in the stream, or not? And there are two <br>
ways to manage this: either use the existing model to store your state, <br>
or track the state in your transform object.<br>
<br>
The first way is simpler in that you can get someone else (the model) to <br>
do your state management, but feels a little distasteful. What you do is:<br>
<br>
- have the accept method always ignore the list-carrier element (e.g., <br>
NestMembers)<br>
- have the atEnd method query builder().original() to see if the <br>
list-carrier element is present (e.g., findAttribute()); if it is <br>
present, get it and transform it and send it downstream, and if is not <br>
present, generate a new one.<br>
<br>
new ClassTransform() {<br>
void accept(ClassBuilder b, ClassElement e) {<br>
switch (e) {<br>
case NestMembersAttribute a -> break;<br>
... other transform stuff ...<br>
}<br>
}<br>
<br>
void atEnd(ClassBuilder b) {<br>
Optional<NestMembersAttribute> a = <br>
b.original().get().findAttribute(Attributes.NEST_MEMBERS);<br>
if (a.isPresent()) {<br>
// transform a, send it to builder<br>
}<br>
else {<br>
// make new attribute, send it to builder<br>
}<br>
}<br>
}<br>
<br>
This is straightforward enough, but it feels a little disconnected.<br>
<br>
The other way involves a _stateful_ transform, where there is state in <br>
the transform class. This feels a little more honest, but it means you <br>
have to use the XxxTransform::ofStateful factory. Here, you transform <br>
elements as you go, and you record whether you saw the NestMembers <br>
attribute, and if you didn't, you emit a fresh one in the atEnd:<br>
<br>
<br>
() -> new ClassTransform() {<br>
boolean foundNM = false;<br>
<br>
void accept(ClassBuilder b, ClassElement e) {<br>
switch (e) {<br>
case NestMembersAttribute a -> { transform a; foundNM = <br>
true; }<br>
... other transform stuff ...<br>
}<br>
}<br>
<br>
void atEnd(ClassBuilder b) {<br>
if (!foundNM)<br>
builder.accept(NestMembersAttribute.of(...));<br>
}<br>
}<br>
<br>
> I ended up reading the existing members, creating a new attribute from<br>
> the existing & new members, then replacing the entire attribute with<br>
> the new one. It was easy to do with a composed transform (very nice<br>
> by the way!), but my code feels ugly in this case - is there a better<br>
> way to do an update of an attribute like this?<br>
<br>
Does the second example above seem better?<br>
<br>
> There are several ::of methods on NestMembersAttribute that help but<br>
> it might be worth adding a ::withSymbols(NestMembersAttribute,<br>
> ClassDesc... additions) api. I can create a PR if there's interest in<br>
> these kinds of additions.<br>
<br>
Yes, we are interested in usability feedback like this. A PR is a good <br>
place to start. I am interested in ensuring that we apply some <br>
relatively uniform principles across the API about what factories we <br>
provide, though, to avoid whack-a-mole in the future. So if we are <br>
missing some overloads for NestMembersAttribute::of, we're probably also <br>
missing the same in Interfaces and other places too.<br>
<br>
> One of the areas I was slightly concerned about was object allocation.<br>
> I haven't measured it but using the APIs - especially mixing<br>
> ClassEntry & j.l.constant.ClassDesc - feels like it generates a lot of<br>
> temporary objects to get a set of entries into a consistent format.<br>
> I needed to convert from one format to the other to be able to create<br>
> new attributes - ie: read the set of ClassEntries and then convert<br>
> them to ClassDesc so they fit with the ClassDesc I already had in<br>
> hand. The ::with* apis additions might help here as well? Or maybe<br>
> there's a better pattern for what I'm trying to do. Guidance<br>
> appreciated.<br>
<br>
Clearly we've opted into a higher degree of cost relative to ASM on <br>
this. (On the other hand, we picked up a lot both from laziness and <br>
from constant pool sharing, so some of this cancels out.) The basic <br>
principle is that there are both "close to the format" and "convenience" <br>
representations for both things, such as ClassEntry and ClassDesc. (We <br>
struggled over whether to admit strings at all, and are currently biased <br>
against using strings for anything other than names, because anyone who <br>
has used ASM a lot has made the "com/foo/Bar" vs "com.foo.Bar" vs <br>
"Lcom/foo/Bar;" mistake at least a hundred bajillion times.) But a <br>
ClassDesc is more expensive to make and manipulate than a string.<br>
<br>
So the advice is to get things into the most close-to-the-representation <br>
format as early as possible (e.g., ClassEntry). And ClassEntry has both <br>
asSymbol() (a ClassDesc) and asInternalName() (a String), so you don't <br>
have to go through ClassDesc when transforming. Adam is also adding <br>
ClassDesc::fromInternalName as an alternate factory to ClassDesc.<br>
<br>
My sense is that we have not quite finished rationalizing <br>
representations when it comes to transforming. If you find yourself <br>
having to convert a lot of stuff to ClassDesc and back when <br>
transforming, either there's a better way to do it, or we should expose <br>
a better way to do it. Hopefully one that doesn't leave users in the <br>
position of guessing "what kind of string is this."<br>
<br>
> Composable ClassTransforms are a very nice addition in the API. After<br>
> writing the code that dropped an attribute, and replaced it with an<br>
> updated attribute, I was able to lift that into a composed transform<br>
> that could be reused in several places.<br>
<br>
This was the holy grail I was seeking, and it took several iterations of <br>
the API to get there. I'm glad we got there.<br>
<br>
<br>
</blockquote></div>