Skip to content

Issue #2116: Catalog initialization from plugin leads to lots of contention#2120

Draft
nick-at-finix wants to merge 1 commit intokillbill:masterfrom
nick-at-finix:issue-2116-prototype
Draft

Issue #2116: Catalog initialization from plugin leads to lots of contention#2120
nick-at-finix wants to merge 1 commit intokillbill:masterfrom
nick-at-finix:issue-2116-prototype

Conversation

@nick-at-finix
Copy link
Copy Markdown
Contributor

Different approach from the previous pull request, completely eliminates contention in CatalogSafetyInitializer for our usages.

@nick-at-finix nick-at-finix force-pushed the issue-2116-prototype branch from 9e48ef4 to 08b7874 Compare May 6, 2025 17:40
Copy link
Copy Markdown
Contributor Author

@nick-at-finix nick-at-finix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments:


private static final Map<Class<?>, LinkedList<Field>> perCatalogClassNonRequiredFields = new HashMap<>();
// Cache of prototype instances with default values already set, avoids repeated reflection
private static final Map<String, Object> prototypeCache = new ConcurrentHashMap<>();
Copy link
Copy Markdown
Contributor Author

@nick-at-finix nick-at-finix May 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a lot faster to hash a String instead of an entire Class. Also - this collection never gets larger than 23 entries in our testing, so that size of this Map should not be a concern.

/**
* Applies values from the prototype to null fields in the target object for minimal reflection usage.
*/
private static void applyPrototypeValues(Object prototype, Object target) throws IllegalAccessException {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This avoids all subsequent annotation checking, value determinations, and type checks for a given Catalog Object.

private static final Map<String, Object> prototypeCache = new ConcurrentHashMap<>();

// Cache for zero-length arrays
private static final Map<Class<?>, Object> zeroLengthArrayCache = new ConcurrentHashMap<>();
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to keep creating these either, there are 9 array types that end up in here.

*/
private static void applyPrototypeValues(Object prototype, Object target) throws IllegalAccessException {
for (final Field field : target.getClass().getDeclaredFields()) {
if (!field.trySetAccessible()) {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Java 9+ has Field#trySetAccessible, which restricts the change in access to the calling method, and doesn't require us to change it back.

@nick-at-finix
Copy link
Copy Markdown
Contributor Author

nick-at-finix commented May 6, 2025

If this approach is generally acceptable I will clean this up, add some logging, and additional checks.

@nick-at-finix nick-at-finix marked this pull request as ready for review May 6, 2025 18:04
@nick-at-finix
Copy link
Copy Markdown
Contributor Author

Hm, over time this is creating a large number of fields:

tomcat@killbill-78f9c49cbf-4fbpm:~$ jmap -histo 1024 | head
Picked up JAVA_TOOL_OPTIONS: -Xms4G -Xmx14G
 num     #instances         #bytes  class name (module)
-------------------------------------------------------
   1:       6968230      501712560  java.lang.reflect.Field (java.base@11.0.26)
   2:       1867911       74716440  java.util.TreeMap$Entry (java.base@11.0.26)
   3:       1419893       68154864  java.util.TreeMap (java.base@11.0.26)
   4:       1250399       62770992  [Ljava.lang.Object; (java.base@11.0.26)
   5:       1241651       59599248  org.killbill.billing.catalog.DefaultPlanPhase
   6:        631985       56406208  [B (java.base@11.0.26)
   7:        174948       55605752  [I (java.base@11.0.26)
   8:       1583857       54595432  [Ljava.lang.reflect.Field; (java.base@11.0.26)

I will iterate on this approach and improve

@nick-at-finix nick-at-finix marked this pull request as draft May 6, 2025 18:21
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.

1 participant