-
-
Notifications
You must be signed in to change notification settings - Fork 925
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
Truffle perf warnings #4395
Truffle perf warnings #4395
Conversation
chrisseaton
commented
Dec 18, 2016
- Fold performance warnings in logging
- When something isn't optimised, warn about it only if it gets compiled, otherwise they probably won't worry that it's slow anyway
- With logging levels we don't need an option to disable performance warnings
Cheers, IntelliJ.
if (CompilerDirectives.inCompiledCode()) { | ||
performance(message); | ||
} | ||
} |
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.
Do we need this one? It seems unused so far.
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.
We're not using it at the moment. I just added it as another option. Do you think I should remove until we actually use it?
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.
Yes, it's trivial to add if we need it and I'd rather avoid extra methods we don't currently need.
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.
Done.
// This isn't double-checked locking, because we aren't publishing an object that is then used | ||
|
||
if (!displayedWarnings.contains(message)) { | ||
synchronized (displayedWarnings) { |
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.
if (displayedWarnings.add(message))
is simpler and should not cause a performance difference I think.
Or maybe use putIfAbsent()
on the Map directly if you want more control.
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.
Fixed.
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.
Looks good, just the synchronization could be simpler.
How does one enable/disable these warnings? |
… to it from configuration.
I added a sample logging configuration file. It's a little fussy, I'm afraid, but it's the standard Java way. https://github.com/jruby/jruby/pull/4395/files#diff-790cdfd51b2ae9a0f85753a2a80d02d3 The great thing about using the standard API is that you can change a few configurations and have us play nicely with any other Java framework you might be using. |
*/ | ||
@TruffleBoundary | ||
public static void performanceOnce(String message) { | ||
// This isn't double-checked locking, because we aren't publishing an object that is then used |
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.
The comment can go as well now :)
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.
Fixed.
Merged! |