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

HHH-13343 : Bytecode enhancement using ByteBuddy fails when the class is not available from the provided ClassLoader #2825

Merged
merged 3 commits into from Apr 5, 2019

Conversation

gbadner
Copy link
Contributor

@gbadner gbadner commented Mar 28, 2019

@gbadner gbadner requested a review from gsmet March 28, 2019 21:38
@@ -143,8 +140,8 @@ public EnhancerImpl(final EnhancementContext enhancementContext, final ByteBuddy
}
Copy link
Member

Choose a reason for hiding this comment

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

Should the Map<String,Resolution> in classFileLocator, be cleared after each enhance(String className, byte[] originalBytes) call to avoid keeping the 'originalBytes' in memory, for large deployments?

Does the transformation of entity classes involves following (class) references to other entity classes? If yes, then we probably cannot clear the Map<String,Resolution>, since they will be needed for the transformations that span multiple classes being referenced by ByteBuddy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@scottmarlow, good questions. I was wondering about your second point as well.

Copy link
Member

Choose a reason for hiding this comment

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

IMO, if Hibernate is enhancing application entity class named 'A' that is associated with class 'B', regardless of which class is loaded/defined/enhanced first, when the other class is enhanced, the Map<String, Resolution> in ClassFileLocator, would be the (input) unenhanced class, instead of the enhanced (returned) class. I think it is likely that we can clear the Map after each call to enhance(), since we wouldn't want to use the (unenhanced) entity class. Also it is unlikely that enhancing more than the class that is passed to the enhance() method, at the same time, would work (instead we should only enhance each class separately.

Copy link
Member

@scottmarlow scottmarlow left a comment

Choose a reason for hiding this comment

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

I think this change will be ready after we are clearing the Map contents, at the tail end of the enhance() call, as per my comments.


private class EnhancerClassFileLocator extends ClassFileLocator.ForClassLoader {

private Map<String,Resolution> nameToResolutionMap = new HashMap<>();
Copy link
Member

Choose a reason for hiding this comment

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

I suppose that instead of clearing the Map, we don't really need a map, but to instead just keep the name + bytes set during the ctor and create a new EnhancerClassFileLocator for each call to enhance(). The instead of nameToResultionMap.get(name), we could just see if saved name is equal to the name parameter, if yes, return new Resolution.Explicit( bytes).

@scottmarlow
Copy link
Member

So, I'm now having second thoughts about my comments from yesterday about not needing the Map collection. I think we would need to try that change against the WildFly tests to make sure they pass.

@gbadner
Copy link
Contributor Author

gbadner commented Apr 5, 2019

@scottmarlow, IIUC, EnhancingClassTransformerImpl#transform calls BytecodeProvider#getEnhancer, which creates a new EnhancerImpl instance with a new EnhancerImpl#classFileLocator. IOW, the EnhancerClassFileLocator instance does not get reused the next time a class is transformed.

Assuming this is what is expected, I agree that there is no need for EnhancerClassFileLocator#nameToResolutionMap; we only need the class name for what is getting transformed and its Resolution. I will go ahead and add a commit that makes this change.

… is not available from the provided ClassLoader
@gbadner
Copy link
Contributor Author

gbadner commented Apr 5, 2019

@scottmarlow, I pushed a commit that removes the Map.

@gbadner
Copy link
Contributor Author

gbadner commented Apr 5, 2019

Travis failures are due to https://hibernate.atlassian.net/browse/HHH-13357.

Copy link
Member

@scottmarlow scottmarlow left a comment

Choose a reason for hiding this comment

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

Looks good, we are passing the https://issues.jboss.org/browse/WFLY-11891 tests now!

@gbadner gbadner merged commit 0506b4a into hibernate:master Apr 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants