Skip to content

Conversation

@rbalamohan
Copy link

DurationInfo is in hotpath in S3A and it would be good to lazy init formatted text. https://issues.apache.org/jira/browse/HADOOP-16751 has the profiler output.

@hadoop-yetus
Copy link

💔 -1 overall

Vote Subsystem Runtime Comment
+0 🆗 reexec 0m 0s Docker mode activated.
-1 ❌ docker 0m 10s Docker failed to build yetus/hadoop:104ccca9169.
Subsystem Report/Notes
GITHUB PR #1754
JIRA Issue HADOOP-16751
Console output https://builds.apache.org/job/hadoop-multibranch/job/PR-1754/1/console
versions git=2.17.1
Powered by Apache Yetus 0.11.1 https://yetus.apache.org

This message was automatically generated.

@steveloughran
Copy link
Contributor

well spotted perf issue.

The patch is doing lazy eval, but it now does it in: start, finish and toString calls, so at least 2x current overhed.

better: just have a String text field which is set to the formatted result if the output is to be logged which is

(logAtInfo && log.isInfoEnabled() || log.isDebugEnabled()

set that in the constructor and reference as needed,

@rbalamohan
Copy link
Author

Thanks for the review Steve.

It would be evaluated only once on need basis (textStr takes care of that).

Having String text based on (logAtInfo && log.isInfoEnabled() || log.isDebugEnabled(), it would fail in some cases. E.g invoking toString on new DurationInfo(log, false, "test") would end up with "null" for text.

@steveloughran
Copy link
Contributor

I see. and reviewing it, yes, that getFormattedText() does update the cached value.

+1

@steveloughran
Copy link
Contributor

merged to trunk; thanks

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.

3 participants