Cache related issue with class definitions about cglib HOT 59 CLOSED

@martin-g , can you please try removing setNamingPolicy? https://github.com/apache/wicket/blob/7973959f169a1c44c1de18a6acde21c8b926d64c/wicket-jmx/src/main/java/org/apache/wicket/jmx/Initializer.java#L290
It looks like it generates duplicate names.

from cglib.

Thanks, @vlsi !
This was the problem indeed!

from cglib.

@martin-g , technically speaking, setSuperClass affects which classloader is used by generated-by-cglib class: https://github.com/apache/wicket/blob/7973959f169a1c44c1de18a6acde21c8b926d64c/wicket-jmx/src/main/java/org/apache/wicket/jmx/Initializer.java#L258

so you might want to call e.setClassLoader to use wickett-related class loader (e.g. for allowing the generated classes to be unloaded on redeploy kind of scenarios)

Currently you are generating classes into Object.class.getClassLoader().

from cglib.

I have this fixed locally!
It was the first thing I've tried myself to resolve this issue.

Now I face some JMX problems - the created proxy is not a proper MBean, but this seems to be some magic with the old naming. I'll figure it out!

Thanks again for your help!

from cglib.

@vlsi, this looks like a regression from the last release. Can we fix it so people's setups don't need to change?

from cglib.

(Is it an issue of just inferring the classloader from the superclass, rather than from Object, if someone sets a superclass w/o setting a classloader?)

from cglib.

Well, one option is to use git bisect to identify which particular commit did flip the behavior.

@martin-g , do you have a small reproducer?

from cglib.

It is not small but it is not big too.

1. git clone https://github.com/apache/wicket
2. mvn clean install (do this once to have the modules in ~/.m2/repository)
3. Edit /pom.xml to change the version of CGLIB
4. mvn install -N && (cd wicket-examples && mvn test)

You need to repeat steps 3-4. It would take you ~30secs to run.

from cglib.

Is it an issue of just inferring the classloader from the superclass, rather than from Object, if someone sets a superclass w/o setting a classloader

I do not think the inference rules has been changed. I've absolutely no idea what the issue could be. Naming policy that always returns the same name just was a blind guess.

from cglib.

The class names are different. The naming policy just removes .wrapper from the package name. (I have no idea why. This code in Wicket is older than me. But I'll debug more tonight.)
The same code works fine with CGLIB 3.2.0. It starts breaking with 3.2.1.

from cglib.

ok dug in. the problem is that wicket's naming policy never called predicate.evaluate. this CL changed the behavior: 05f830f . previously it was all synchronized so the code would ask the policy for a name & then add to it a cache. now it's concurrent-friendly, so the act of getting a name adds it to the cache (but only if getting the name asked the predicate if it was OK).

since wicket never asked the predicate if it was OK, the cache never adds the new field. this may have been safe before if the dev knew 100% that the name was OK.

we should probably defensively add the resulting name to the cache. it won't be 100% perfect because someone could concurrently steal the name, but it wouldn't have been safe before either since the naming policy never checked.

from cglib.

alternately, we synchronized block around name generation. @vlsi, do you think if we added a synchronized block just around the name generation aspect (

from cglib.

@sameb , I'm still not convinced how synchronized helps to generate unique names provided the code (I mean wicket) does not consult with the set of occupied names.

we should probably defensively add the resulting name to the cache

That makes sense (even though I do not get if that solves particular issue or not)
Current logic of "allocate name in predicate" is better to be split into

1. reserve name in predicate
2. definitely allocate it after generateClassName <-- this one is missing
3. bailout if generateClassName generates non-unique name (that is the name that has already been generated for the same classloader)

from cglib.

assuming all naming policies check the predicate, synchronized won't make any difference.
but if there's a naming policy that doesn't check the predicate, then adding it afterward w/o synchronized is "close but no cigar".

imagine there's two policies: X & Y. X checks the predicate and Y doesn't. given the flow:
step 1: thread 1 - Y uses name "Foo"
step 2: thread 1 - add "Foo" to cache
step 3: thread 2 - X checks if "Foo" is available and sees it isn't, generates "Foo1" instead.

that works fine because step 2 happened before step 3. now imagine that thread 1 got preempted and step 3 happens before step 2. it won't know that "Foo" is taken, so X will try to use "Foo" and fail.

from cglib.

Well, what if step 3 happens before step 1?
X takes Foo (it is free), then comes Y ant takes "Foo" (as it always uses Foo).

synchronized won't help there.

There is if (attemptLoad) { Class.forName(generatedName)} kind of check, however I do not think it is safe to load "created by someone else class that just happen to have the same name".

I mean attemptLoad works if X and Y mean exactly the same thing for name Foo.
If they mean different bytecodes, then they both are in trouble.

Does that make sense?

from cglib.

if step 3 happens before it would break, but it would have been broken before anyway. no change there.

from cglib.

basically the goal here is to make sure that broken policies (those that don't look at the predicate) don't break correct policies. we can't fix broken policies, but we can limit their damage.

from cglib.

but we can limit their damage

Is this step1 step3 step2 thing with X and Y that exactly happens with wicket?

from cglib.

There is no step 2 today, so really all it needs is step 1 then step 3.

On Wed, Jun 15, 2016, 11:56 AM Vladimir Sitnikov [email protected]
wrote:

but we can limit their damage

Is this step1 step3 step2 thing with X and Y that exactly happens with
wicket?

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#75 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/ACC7EnlQAOHeb6fXbj62kS4UC-c_9Ixeks5qMCCegaJpZM4I2FFa
.

from cglib.

I mean: are there X and Y? (different name generators that somehow end up with exactly the same name, and one of them uses the predicate while the other does not)

from cglib.

No idea. There may be something else wrong too, but from analyzing the
code, this is one problem I found.

On Wed, Jun 15, 2016, 12:01 PM Vladimir Sitnikov [email protected]
wrote:

I mean: are there X and Y? (different name generators that somehow end up
with exactly the same name, and one of them uses the predicate while the
other does not)

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#75 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/ACC7Eg5QsmRVgTOzwy2gliDNiIu9k9Tkks5qMCHHgaJpZM4I2FFa
.

from cglib.

Let me clarify: I think adding synchronized over name generation would not be visible performance-wise. There is pretty much the same synchronization for defineClass a bit later, and AbstractClassGenerator.generate is called only in case the class has not yet been generated.

However, I'm still trying to understand if that is throw ConcurrentModificationException kind of condition, when it is better to throw an exception (or even use alternative name) rather than silently ignoring the fact that predicate was not used..

from cglib.

cglib never posed any kind of requirement that policies needed to check the predicate. it was good for them to do so because if they didn't they might get an error, but not doing so never broke the state for other parts of the program. i don't think we should suddenly make it an error to not use the predicate.

given that -- if you think it's OK to add synchronized (becaues defineClass has a synchronize later anyway), can you clarify why 05f830f was a helpful CL? if there's already guarantee synchronization (and if we're going to be adding more), why try and make just a small portion of it concurrent, since it'll end up synchronizing anyway?

from cglib.

can you clarify why 05f830f was a helpful CL?

It helps by avoiding synchronization in case the class has already been generated for the same key.

Generally speaking, I do not care how much time does it take to actually generate bytecode as the proper usage should reuse classes, thus it should just fetch relevant class out of the cache and skip the generation altogether.

Infinite number of classes is not good for JVM performance.

That is why I say "adding synchronization" over class name generation would not hurt much. It does hurt, however it impacts only the cases that end up in bytecode generation (that is inherently slow).

On the other hand, the more the number of syncrhonized sections is, the more deadlock is likely to happen, so I just confined synchronization to the minimum.

from cglib.

ok i see. if the key to create(Key) is the same it should avoid going into the generate(..) method and fully avoid synchronization. if it's different it's OK to synchronize. makes sense, i missed that part. (i'm digging into another bug we're seeing right now where for some reason the same class is being regenerated even w/o using any custom naming policy, so am taking a close look at this code.)

from cglib.

Just in case, the following does NOT solve wicket's issue:

            synchronized (classLoader) {
                String className = generateClassName(data.getUniqueNamePredicate());
                this.setClassName(className);
                data.getUniqueNamePredicate().evaluate(className);
            }

Maybe they're seeing something similar to what we're seeing. My current
hunch is the LoadingCache is doing something wrong. Digging in there now.

On Wed, Jun 15, 2016, 1:53 PM Vladimir Sitnikov [email protected]
wrote:

Just in case, the following does NOT solve wicket's issue:

        synchronized (classLoader) {
            String className = generateClassName(data.getUniqueNamePredicate());
            this.setClassName(className);
            data.getUniqueNamePredicate().evaluate(className);
        }

—
You are receiving this because you commented.

Reply to this email directly, view it on GitHub
#75 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/ACC7Ej5BOhEuabl0ppNoypEE_GOFBdCDks5qMDwMgaJpZM4I2FFa
.

from cglib.

found the problem. AbstractClassGenerator.create is missing a "cache = CACHE" line at the top of the synchronized block. Without it, the double-checked locking is incorrect. Multiple threads can see data == null outside the block & then wait on the synchronized block, and then by definition it will still be null in the block since we never rewrote the local cache variable. Once I confirm this fixes it, I'll send a PR with this fix and the reserve-classnames fix.

from cglib.

yeah that fixes it.

from cglib.

#78 out with the fix.

from cglib.

@sameb , ef9ccb4 does not solve the issue. Please reopen.

from cglib.

@sameb , here's the fix for wicket's issue: #79
I'll add test case a bit later (however it should be as simple as enhancer with naming policy that always returns the same name).

The problem was cglib failed to create Factory class for the proxy class in question as the name policy for the factory class resulted in exactly the same name, thus the failure.

The factory in question is internal, so cglib can pick whatever naming policy is useful.

from cglib.

not sure why you want me to revert. wicket may have a different issue, but both things solved in that change are real problems that need fixing.

from cglib.

I should have said "reopen" not "revert".

from cglib.

ok so #78 looks good to you? can you explain why #79 is necessary? what was the earlier change that broke things and made it necessary?

from cglib.

Here's the problematic change:
05f830f#diff-173a9c4b512f39086ed564b6320318e2R629

Previously, cglib always used reflection to instantiate the proxy.
As we all know, reflection is slow, so I refactored it to creating a specialized Factory class behind the scenes.

E.g. wicket asks for a proxy class org/apache/wicket/jmx/Application that implements just net/sf/cglib/proxy/InvocationHandler. cglib tried to create org/apache/wicket/jmx/ApplicationFactory$BYCGLIB$123123 behind the scenes so org/apache/wicket/jmx/Application can be created faster.

As naming policy was not using the predicate, the factory got named exactly as the proxy class, thus the collision.

This is how #79 works. It resets naming policy to known-to-be-working policy for the duration of factory generation.

from cglib.

interesting ok. i will redo w/ a test locally.

from cglib.

@vlsi i have a few more questions:

1. if i understand correctly, the useFactory var (Enhancer.setUseFactory) used to control whether or not the generated class implicitly implemented Factory. it defaulted to true, and so the first generated instance would be the factory for creating every next instance. is that correct?
2. if (and only if) the user called setUseFactory(false) and they didn't manually say to proxy Factory, then cglib resorted to using reflection for creation. is that correct?
3. after your changes, it looks like we will always generate a new Factory class for each Enhanced class, regardless of if the user called setUseFactory(false) or if they said to implement Factory.class. is that correct?

if so, while your PR fixes the issue, i think we have larger problems: forcibly creating a new Factory class for every Enhanced class is a huge change in what gets generated. especially in the default case where the proxied class was going to implement Factory itself anyway. along the same lines, even if the user called setUseFactory(false), forcing 2 code-gen'd classes for each Enhanced class seems bad to me.

am i understand things correctly or did i get something wrong somewhere?

from cglib.

so the first generated instance would be the factory for creating every next instance.

This behavior was reverted here: 880f7b3#diff-13ef5fff6889a608d911b20abda8e76fL220.

Storing a stateful proxy object in cglib cache might introduce memory leaks (e.g. unwanted strong references to callbacks, etc). In my point of view that was the driver to remove firstInstance cache.

That was the main driver to "always generate factory class".

forcing 2 code-gen'd classes for each Enhanced class seems bad to me

What is bad? Can you elaborate?
If cglib manages to defineClass for proxy-class, then it will almost likely to succeed loading a factory class.

For one-use proxy class performance does not matter. For hot-used proxy classes, factory speeds up execution dramatically.

We can explore MethodHandle way of instantiating, however that is not available on older JVMs.

Even if reflection is used for instantiation, it will end up generating accessor class by the JVM itself. Hot reflection objects are generated into classes, thus the number of classes would still be "at least 2 per proxy class".

from cglib.

Is it not possible to instantiate the same way FastClass does? Isn't the whole point of that to speed up instantiation (doing so without a second Factory class)?

The reason two code genned classes are bad is because prior to java8 it will permanently use up PermGen space, and after java8 it uses up other space. I'm not sure if reflective instantiation has the same effect, but I don't think it does?

I'm confused with the code though -- before these changes, what was the point of adding the Factory interface if we never kept the instantiated object to use it? And after the recent changes, what's the point of it at all?

from cglib.

Is it not possible to instantiate the same way FastClass does?

FastClass-generated object extends FastClass, thus it uses net.sf.cglib.reflect.FastClass#newInstance(int, java.lang.Object[]) to instantiate.
There is no way to reuse that for user-provided proxy classes that might extend from arbitrary class.

The reason two code genned classes are bad is because prior to java8 it will permanently use up PermGen space

This is false. Classes can be unloaded as classloader gets unreachable.

reflective instantiation has the same effect, but I don't think it does?

Hot-used reflection does generate classes for faster access. So using reflection would not reduce the number of generated classes.

was the point of adding the Factory interface

Factory was there event before I saw cglib for the first time.
I think the point of Factory was to enable users to "clone" proxy-classes faster, without resorting to Enhancer/bla/bla/bla/create.

from cglib.

You assume a classloader can get unloaded. That only happens in environments where multiple classloaders are used.

You say "hot use reflection generates classes". Does that mean infrequently used ones do not? If that's true, it seems like reflection is a much better option. It evaluates if classes are necessary, and if so, generates them.

from cglib.

If that's true, it seems like reflection is a much better option.

cglib is often advertised to be "fast", however it cannot be fast if it uses the same reflection.

In any case, the generated factory is a small wrapper over newInstance(Object[] args), so its class size & footprint on permgen are close to negligible.

#2 imposes much higher tax on permgen and heap than a simple class with single newInstance method.

from cglib.

Just a little note from the side: Reflection is fast, the JVM uses code generation internally to avoid JNI since Java 5 ("inflation"). What is costly however is the reflective lookup as it involves allocation and security checks. Otherwise, I benchmarked reflection vs. code generation and it is equally efficient, even for primitive types if the VM can apply proper type erasure.

from cglib.

Is it possible to avoid the reflective lookup (or cache it?) yet still use reflection for the creation?

from cglib.

Otherwise, I benchmarked reflection vs. code generation and it is equally efficient.

@raphw , Please share the results. Reflection does security checks upon execution, not upon "lookup"
I think you confuse reflection vs methodHandles, do you?

Here are the measurements by Alexey (you-know-who): http://stackoverflow.com/a/22337726/1261287
Reflective set is 2.3 times slower than plain access in 8u40. In older JVMs it would get even worse.

from cglib.

I was not precise in my answer. In this case, the constructor is made visible by cglib such that the checks do not apply. Here is an overview of my benchmarks for the general case: https://gist.github.com/raphw/881e1745996f9d314ab0

The lookup costs come mostly from the need to copy the Method or Constructor instance as those objects are mutable.

from cglib.

So my take here is that reflection will be OK and not expensive? We should have the class definition already handy from creating it. If the cost is reasonable (which it sounds like it is), reflection seems a much better option since it generates classes only if it thinks it'll be helpful.

from cglib.

If "instantiation via reflection" can be made faster or comparable vs bytecode-generated Factory, then it could makes sense to rewrite Factory to reflection as well.

Otherwise cglib will end up with two approaches that might differ in behavior.

from cglib.

I'm all for removing redundant approaches that differ so long as the result is simple, straightforward, and works correctly without drawbacks.

from cglib.

that was one of the reasons I just reused existing Factory machinery instead of adding some new reflection-based factory/cache.

from cglib.

Was the factory mechanism actually used anywhere? IIUC, it seemed like it was effectively unused (because 10 years ago they made a change to not store the stateful proxy)? The previous setup seems to have always used reflection. Changing that setup to instead always generate a 2nd factory class doesn't seem like a net win to me, especially if we can show that reflection worked just as well.

from cglib.

because 10 years ago they made a change to not store the stateful proxy

net.sf.cglib.proxy.Factory was a part of public API for quite a while. Do you suggest to drop it?

from cglib.

No, but that's a different story. cglib itself didn't use it -- it just let people opt into it or cast to it to create more of the same. I'd rather not change how cglib created new instances if there was nothing wrong with it in the first place.

from cglib.

@raphw , did you benchrmark reflection in java 1.6 and 1.7? I think cglib should still support those and do not regress much.
Here's what I get in OracleJDK 1.7u55:

Benchmark                                                             Mode  Cnt     Score      Error   Units
InvocationBenchmark.normal                                            avgt    5    30,581 ±   15,247   ns/op
InvocationBenchmark.normal:·gc.alloc.rate.norm                        avgt    5    64,000 ±    0,001    B/op
InvocationBenchmark.primitive                                         avgt    5     2,873 ±    0,105   ns/op
InvocationBenchmark.primitive:·gc.alloc.rate.norm                     avgt    5    ≈ 10⁻⁶               B/op
InvocationBenchmark.privateNormal                                     avgt    5    28,778 ±    2,278   ns/op
InvocationBenchmark.privateNormal:·gc.alloc.rate.norm                 avgt    5    64,000 ±    0,001    B/op
InvocationBenchmark.reflection                                        avgt    5   276,431 ±   12,857   ns/op
InvocationBenchmark.reflection:·gc.alloc.rate.norm                    avgt    5    96,000 ±    0,001    B/op
InvocationBenchmark.reflectionAccessible                              avgt    5    37,228 ±    1,554   ns/op
InvocationBenchmark.reflectionAccessible:·gc.alloc.rate.norm          avgt    5    96,000 ±    0,001    B/op
InvocationBenchmark.reflectionAccessiblePrimitive                     avgt    5    30,393 ±    2,022   ns/op
InvocationBenchmark.reflectionAccessiblePrimitive:·gc.alloc.rate.norm avgt    5    48,000 ±    0,001    B/op
InvocationBenchmark.reflectionAccessiblePrivate                       avgt    5    37,318 ±    2,971   ns/op
InvocationBenchmark.reflectionAccessiblePrivate:·gc.alloc.rate.norm   avgt    5    96,000 ±    0,001    B/op
InvocationBenchmark.reflectionPrimitive                               avgt    5   258,634 ±   19,448   ns/op
InvocationBenchmark.reflectionPrimitive:·gc.alloc.rate.norm           avgt    5    48,000 ±    0,001    B/op

If things are OK on java8+, I'm happy. Creating a new class because old java versions aren't as fast a newer ones doesn't sound like a good tradeoff to me. If folks are using old environments, oh well.

from cglib.

@sameb , @raphw regarding reflection, there's "dispatch" part.
net.sf.cglib.core.ReflectUtils#newInstance(java.lang.Class, java.lang.Class[], java.lang.Object[]) dispatches across different constructors, thus a factory might specialize better.

Frankly speaking, I still think #79 is the proper way to go.

from cglib.

I've just tested with 3.2.4-SNAPSHOT and everything seems to work just fine in Wicket!
Thank you all!

from cglib.

thanks for confirming @martin-g . i've just released cglib 3.2.4 to maven. it may take a few hours for it to show up.

from cglib.