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

perf: guard logging statements #1112

Merged
merged 2 commits into from Jun 30, 2018
Merged

perf: guard logging statements #1112

merged 2 commits into from Jun 30, 2018

Conversation

@jesperpedersen
Copy link
Contributor

@jesperpedersen jesperpedersen commented Feb 13, 2018

Add some more guards to logging statements to make it consistent with other parts of the code

@jesperpedersen
Copy link
Contributor Author

@jesperpedersen jesperpedersen commented Feb 13, 2018

Seems like there is something wrong with the Zulu build

Copy link
Member

@vlsi vlsi left a comment

-1

The guards are required for multi-argument calls.
The only purpose of the guards is to avoid creating Object[] for varargs (==performance reasons).

I think adding explicit if (loggable) makes code harder to read, and I do think it is good when we can avoid extra if checks

@@ -148,7 +148,9 @@ public static Encoding getDatabaseEncoding(String databaseEncoding) {
String[] candidates = encodings.get(databaseEncoding);
if (candidates != null) {
for (String candidate : candidates) {
LOGGER.log(Level.FINEST, "Search encoding candidate {0}", candidate);
if (LOGGER.isLoggable(Level.FINEST)) {
LOGGER.log(Level.FINEST, "Search encoding candidate {0}", candidate);

This comment has been minimized.

@vlsi

vlsi Feb 13, 2018
Member

This is log(Level level, String msg, Object param1), and it does not require guards

@davecramer
Copy link
Member

@davecramer davecramer commented Feb 13, 2018

@jesperpedersen
Copy link
Contributor Author

@jesperpedersen jesperpedersen commented Feb 13, 2018

I agree that the big win is for log statements with arguments, however Logger.log() does an internal if (enabled), so this saves 1 function call per log statement.

Feel free to close.

@davecramer
Copy link
Member

@davecramer davecramer commented Feb 13, 2018

@jesperpedersen
Copy link
Contributor Author

@jesperpedersen jesperpedersen commented Feb 13, 2018

Logger.isLoggable vs Logger.log() + Logger.isLoggable()

@vlsi vlsi force-pushed the jesperpedersen:logging branch from 6c9b8bd to a789065 Jun 30, 2018
@vlsi vlsi added this to the 42.2.3 milestone Jun 30, 2018
@vlsi vlsi changed the title Guard logging statements perf: guard logging statements Jun 30, 2018
@vlsi vlsi merged commit 7a0b7d6 into pgjdbc:master Jun 30, 2018
1 check was pending
1 check was pending
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
rhavermans added a commit to bolcom/pgjdbc that referenced this pull request Jul 13, 2018
Guard certain logging statements with `isLoggable` to prevent `Object[]` allocation for the argument vararg.
rhavermans added a commit to bolcom/pgjdbc that referenced this pull request Jul 13, 2018
Guard certain logging statements with `isLoggable` to prevent `Object[]` allocation for the argument vararg.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.