-
Notifications
You must be signed in to change notification settings - Fork 277
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
avoid creating new strings prior to jar lookup #1580
Conversation
f892c7e
to
a014468
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't see that this would change behavior, since these are the only places that filenameToEntry
is used.
Well, when we shadow, we replace ".class" with ".classdata" and it's not entirely clear to me that the map can only contain our classes. If we take someone else's class and suffix it with "data" prior to lookup and the map contains classes which aren't ours, it would previously miss, but would hit now. Hence the ambivalence despite the obvious benefits. |
I did a small test with pet clinic where the process exits once the agent is installed to check this didn't have negative consequences. On master:
On this branch:
Take it with a pinch of salt, but this doesn't make things worse for the sake of reduced allocation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I think it's unlikely, you're right that there could be other items in the jar that end with "data". The easy fix is to check if the suffix is .classdata
instead
dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/InternalJarURLHandler.java
Outdated
Show resolved
Hide resolved
…strap/InternalJarURLHandler.java Co-authored-by: Laplie Anderson <randomanderson@users.noreply.github.com>
Uses the name we will lookup by as the key in the HashMap during building, which has two effects:
I don't think this changes behaviour, but I'm not entirely sure.