[jira] [Commented] (LOG4J2-812) Performance optimization: avoid use of synchronized SimpleDateFormat in Dat​ePatternCo​nverter

[Gary Gregory (JIRA)]

    [ 
https://issues.apache.org/jira/browse/LOG4J2-812?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14617160#comment-14617160
 ] 

Gary Gregory commented on LOG4J2-812:
-------------------------------------

Looks good. Well done.

Comments/Requests on 
https://issues.apache.org/jira/secure/attachment/12743847/LOG4J2-812.patch3.diff

- Fix the message {{Could not instantiate SimpleDateFormat with pattern}} to 
mention {{FastDateFormat}}.
- I would make ivars and params like {{timestamp}} and {{time}} specify their 
scale, for example {{timestampMillis}} vs. {{timestampNanos}}.
- Make everything final that can be final.
- Make comments like "Copied from Commons Lang 3" more specific, for example: 
"Copied from Commons Lang 3.4 tag http://..."; or "Copied from Commons Lang 3.4 
Git revision xxx". This will allow maintainers to know when it might make sense 
to look for updating the code from Commons Lang again.

With this implementation, the trade-off is that a new {{CurrentTime}} object is 
created whenever we need to format a new timestamp. It seems like a we would be 
generating a lot of garbage but I do not see an alternative since the patch 
cleverly uses of an {{AtomicReference}}.



> Performance optimization: avoid use of synchronized SimpleDateFormat in 
> Dat​ePatternCo​nverter 
> -----------------------------------------------------------------------------------------------
>
>                 Key: LOG4J2-812
>                 URL: https://issues.apache.org/jira/browse/LOG4J2-812
>             Project: Log4j 2
>          Issue Type: Bug
>          Components: Pattern Converters, Performance Benchmarks
>    Affects Versions: 2.0.2
>            Reporter: Mohit Anchlia
>            Assignee: Remko Popma
>             Fix For: 2.3
>
>         Attachments: LOG4J2-812-patch.txt, LOG4J2-812.patch2.diff, 
> LOG4J2-812.patch3.diff
>
>
> Threads seem to be blocking on class 
> org.apache​.loggin.lo​g4j.core.p​attern.Dat​ePatternCo​nverter. It's short 
> lived but is visible in profiler. It also is adding on to CPU. Here is the 
> mail conversation on the mailing list:
> {quote}
> Ralph Goers ralph.go...@dslextreme.com via logging.apache.org  
> The converter uses a SimpleDateFormat which is not thread safe and so is 
> synchronized. I am sure there might be minor optimizations that could be done 
> to this
>  What I would do is modify DatePatternConverter to
>  a) use Java 8’s java.time.format.DateTimeFormatter if running on Java 8
>  b) use Joda Time’s DateTimeFormat if it is present.
>  c) create a pool of SimpleDateFormat objects and use those.
>  Please create a Jira issue for this.
>  Ralph
> {quote}
> ----
> One alternative that  was suggested on the mailing list is to use commons 
> lang FastDateFormat to format log timestamps.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

---------------------------------------------------------------------
To unsubscribe, e-mail: log4j-dev-unsubscr...@logging.apache.org
For additional commands, e-mail: log4j-dev-h...@logging.apache.org