Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Concurrent cache of generated classes #53

Merged
merged 6 commits into from Feb 5, 2016
Merged

Conversation

vlsi
Copy link
Contributor

@vlsi vlsi commented Nov 14, 2015

This fix removes synchronization in case relevant class is already cached.

The change comes on top of "fix permgen" (it reuses WeakIdentityKey)

Measurements (java 1.8u60, i7-4960HQ CPU @ 2.60GHz):
#1 Thread

Response time is 15 times better.
Heap allocation is 5 times less.

Before:

Benchmark                                       Mode  Cnt     Score     Error   Units
BeansBenchmark.newInstance                      avgt    5  2135,987 ± 262,118   ns/op
BeansBenchmark.newInstance:·gc.alloc.rate.norm  avgt    5  1240,021 ±   0,168    B/op

After:

Benchmark                                       Mode  Cnt     Score     Error   Units
BeansBenchmark.newInstance                      avgt    5   142,438 ±   8,209   ns/op
BeansBenchmark.newInstance:·gc.alloc.rate.norm  avgt    5   256,001 ±   0,012    B/op

Multiple threads

Good scalability in range of 1-2-4 threads.
Degradation after 4 is somewhat expected as the machine has just 4 hw cores.

Before:

Benchmark                   Mode Thr Cnt      Score      Error   Unit
BeansBenchmark.newInstance  avgt   1   5   2518,232 ±  148,755  ns/op
BeansBenchmark.newInstance  avgt   2   5   3207,201 ±  231,017  ns/op
BeansBenchmark.newInstance  avgt   4   5   7842,192 ±  528,633  ns/op
BeansBenchmark.newInstance  avgt   8   5  16380,357 ±  941,975  ns/op

After:

Benchmark                   Mode Thr Cnt      Score      Error   Unit
BeansBenchmark.newInstance  avgt   1   5    143,228 ±   15,594  ns/op
BeansBenchmark.newInstance  avgt   2   5    161,668 ±    2,057  ns/op
BeansBenchmark.newInstance  avgt   4   5    152,933 ±    2,753  ns/op
BeansBenchmark.newInstance  avgt   6   5    242,031 ±   42,862  ns/op
BeansBenchmark.newInstance  avgt   8   5    298,683 ±   39,506  ns/op

Previously KeyFactory generated java.lang.Class fields even with CLASS_BY_NAME customization.
That might lead to unexpected class and classloader leak.
The fix ensures KeyFactory uses java.lang.String to store class names.
It should make no harm as CGLIB uses per-classloader cache, thus class name should be sufficiently unique
@vlsi
Copy link
Contributor Author

vlsi commented Nov 16, 2015

Please, review the PR & approach.

In a couple of words:

  1. I made class generation cache shared across all the Source instances. I think it simplifies code.
  2. When new classLoader is added to the list, then copy-on-write under global lock WeakHashMap is used. I believe it should be very rare. WeakHashMap allows to have weak reference to the ClassLoader while avoiding creation of WeakReference(classLoader) for map.get(...) purpose.
  3. Inside the classloader, ConcurrentHashMap is used, so readers do not block each other (so this is the major improvement over previous plain HashMap)
  4. For repeatable object creation, separate Factory class is created and reused behind the scenes. Regular "proxy" is always created first, then factory is created if needed.
  5. Added Enhancer#createFactory() to easy generation of Factory instance for given configuration.
    Not sure if makes sense adding Enhancer#createFactory(Class type, Class callbackType), Enhancer#createFactory(Class superclass, Class interfaces[], Class callbackType), Enhancer#createFactory(Class superclass, Class interfaces[], CallbackFilter filter, Class[] callbackTypes)

@vlsi
Copy link
Contributor Author

vlsi commented Dec 4, 2015

Anybody there?
Is there a chance of accepting this PR?

raphw pushed a commit that referenced this pull request Feb 5, 2016
Concurrent cache of generated classes
@raphw raphw merged commit ef8fdd4 into cglib:master Feb 5, 2016
@raphw
Copy link
Member

raphw commented Feb 5, 2016

The cache is definetly a pain-point in cglib. This is a real improvement! Request looks good!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants