[External] : Re: ClassHierarchyResolver using Reflection information

[Remi Forax]

For the caching part, it can be easier to ask each implementation to cache it's own results (like a ClassLoader does) given that depending on the implementation, the way to cache is quite different. 

regards, 
Rémi 

> From: "Brian Goetz" <brian.goetz at oracle.com>
> To: "-" <liangchenblue at gmail.com>, "Adam Sotona" <adam.sotona at oracle.com>
> Cc: "Michael van Acken" <michael.van.acken at gmail.com>, "classfile-api-dev"
> <classfile-api-dev at openjdk.org>, asotona at openjdk.org
> Sent: Monday, March 20, 2023 7:19:45 PM
> Subject: Re: [External] : Re: ClassHierarchyResolver using Reflection
> information

> The CHR interface is straightforward enough -- given a symbolic reference to a
> class, provide two bits of information: superclass and is-interface.

> We have at least four sensible implementations, which can be combined:
> - do nothing
> - work from an explicit static list (suitable for compilers who are generating
> classes that do not yet exist)
> - Extract `.class` resources from the running JDK
> - Extract live Class objects from the running JDK

> These go in increasing order of work done and environmental dependency; earlier
> items will be faster and more reproducible; latter items will be more usable.
> The CHR interface also supports chaining with `orElse`, so that we can start
> with one strategy and fall back to another.

> Separately, there is caching, which should be orthogonal but which is built into
> the default resolver. There's a public CachedClassHierarchyResolver wrapper
> (probably should be private) which delegates to another CHR and memoizes the
> result in a synchronized HashMap. (There's a static factory ofCached which
> delegates to this, but it is unused.)

> More importantly, the default adopts the cache strategy of "cache lives forever
> / shared among all clients of the API." I get that we want to avoid
> re-resolving these classes every time, but if we're going to include class
> loading as a way to put elements in the cache, there is a risk that it becomes
> polluted with "private" stale data (and also retains memory forever.)

> So things I would like to fix include:

> - The basic implementations are not in a form that makes them easily reusable;
> - It is not obvious exactly what the default resolver does (and the name may be
> wrong), or when the user would want to select an alternate strategy, or what
> its caching strategy is, but it should definitely be defined in terms of
> building blocks the user understands;
> - The caching strategy of the default is questionable and probably needs more
> configuration.

> We currently define the default as:

> new CachedCHR( cd ->
> ClassLoader.getSystemResourceAsStream(Util.toInternalName(classDesc) +
> ".class"))

> and it is being proposed that it be redefined as that, plus falling back to
> class loading if that fails.

> What I'd like users to come away with is:

> - There's a menu of basic options, which can be combined;
> - There's a way to wrap the basic options with caching, and ideally the user can
> have some control over sharing / lifetime / caching strategy;
> - There's a default, which is defined strictly in terms of the menu above, which
> they can reason about.

> We can start with exposing the basics. We already have CHR.of(interfaces,
> supersMap). We need factories or constants for "do nothing", "system resources
> from classloader X", "live classes from classloader X", etc.

> We can refine caching so that CachedCHR is a private implementation, and all
> public API is funneled through ofCached. It is possible we will want to expose
> some more control over the cache, such as wrapping the whole cache with a
> SoftRef, or allowing the user to specify the backing map; this is one more
> factory.

> I think we should rename DEFAULT_CHR to something like GLOBAL_CHR to reflect the
> fact that it comes with a long-lived, JVM-wide cache. GLOBAL_CHR can be
> specified to be CachedCHR.of(systemResourcesCHR()) or
> CachedCHR.of(systemResourcesChr().orElse(systemClassloaderCHR())).

> We can _then_ define DEFAULT_CHR to point to GLOBAL_CHR, and people who want a
> non-shared cache can substitute one.

> Smaller observation: we have shied away from using public records in the API,
> instead having a public interface and a private record-backed implementation.
> ClassHierarchyInfo is currently a public record.

> Stepping back, the global cache suggests that we might want to refactor the
> front-door entry point to the API. Right now, we have a pile of static methods
> in Classfile (build/parse), many of which take sets of options. The point of
> the global cache is to avoid re-resolving information every time, but the
> global granularity is questionable -- it seems much more likely that an agent
> would want to create a static per-agent configuration, shared across uses of
> that agent, but not shared with other agents. This suggests that perhaps the
> methods on Classfile really should be instance methods on an object that
> encapsulates the option set as well as accumulating cache state. (And if the
> agent is unloaded, all the cache goes with it.)

> Something like:

> public class ClassfileReaderWriterThingie {
> public ClassfileReaderWriterThingie(Option... options) { ... }

> public ClassModel parse(byte[] bs) { ... }
> public ClassModel parse(Path p) { ... }

> public byte[] build(ClassDesc thisClass, Consumer<CB> handler) { ... }
> // drop overload taking options, since its in the ctor
> public byte[] build(ClassEntry thisClassEntry, CPB cpBuilder, Consumer<CB>
> handler) { ... }

> // buildTo and buildModule overloads
> }

> This is a somewhat more intrusive API change (though not too much), so my
> suggestion is to proceed on the above first and then revisit this one.

> (One point being that if we're going to have a long-lived cache, it should
> probably be encapsulated in an object the user can control.)

> On 3/20/2023 12:51 PM, Brian Goetz wrote:

>> After looking over this corner of the API, I think there's a little more design
>> work that needs to go into it. So let's step back from the details of "what's
>> the default" until we have a chance to look a little more holistically at this
>> part of the API. I'll try to write more about it soon.

>> On 3/20/2023 11:47 AM, - wrote:

>>> If the choice of default resolver is still subject to debate, I can
>>> remove the delegation to system class loader based resolver in [
>>> https://urldefense.com/v3/__https://github.com/openjdk/jdk/pull/13082__;!!ACWV5N9M2RV99hQ!K7UWlrg2NHotSKOzMvmfnenKZ1dDuOZ9zXcQvlKOdB21VvwTRIDuI2NyRNK6Hq844XIkt_6Q-qFt5DsFDUzgKJ31$
>>> |
>>> https://urldefense.com/v3/__https://github.com/openjdk/jdk/pull/13082__;!!ACWV5N9M2RV99hQ!K7UWlrg2NHotSKOzMvmfnenKZ1dDuOZ9zXcQvlKOdB21VvwTRIDuI2NyRNK6Hq844XIkt_6Q-qFt5DsFDUzgKJ31$
>>> ] , and we can change it in
>>> another patch once we reach a consensus. The addition of lookup and
>>> class loader based resolvers can be used by users who explicitly
>>> specify a resolver in the options.

>>> On Mon, Mar 20, 2023 at 9:00 AM Adam Sotona [ mailto:adam.sotona at oracle.com |
>>> <adam.sotona at oracle.com> ] wrote:

>>>> New behavior is “no assumptions” and throw IllegalArgumentException if
>>>> information about type necessary to build stack map (or verify stack map) is
>>>> not obtained from the resolver (or better say chain of resolvers).

>>>> If user provides no resolver – the default is used.

>>>> The way how default resolver gets the information from system classloader is
>>>> subject of discussion below (resource parsing and fallback to class loading).

>>>> Custom resolver can be constructed as a fallback chain from all provided
>>>> resolver types (including the default resolver instance).

>>>> On 20.03.2023 14:25, "Brian Goetz" [ mailto:brian.goetz at oracle.com |
>>>> <brian.goetz at oracle.com> ] wrote:

>>>> If the user provides no resolver at all, what is the new behavior?  Do we just
>>>> assume Object is the common supertype for any pair of classes?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/classfile-api-dev/attachments/20230320/e8c29b3e/attachment-0001.htm>