Re: [LOG4J2-3672] Time zone parsing in `FastDateParser`

[Volkan Yazıcı]

Done.

On Thu, Nov 2, 2023 at 6:58 PM Piotr P. Karwasz 
wrote:

> Hi Volkan,
>
> On Thu, 2 Nov 2023 at 18:50, Volkan Yazıcı  wrote:
> >
> > I guess the safest alternative would be to lazily initialize the
> > `FastDateFormat#parser` field.
>
> I am happy we agree on the right solution, respectful of the public
> API! Now you have my blessing to shred those unused classes to pieces.
>
> Can you also cherry-pick it to `main`?
>
> Piotr
>

Re: [LOG4J2-3672] Time zone parsing in `FastDateParser`

[Volkan Yazıcı]

I guess the safest alternative would be to lazily initialize the
`FastDateFormat#parser` field. Nevertheless, I do not want to miss this
deprecation unicorn, and hence, deleted the code

and
some tests

.

I ask for your understanding and forgiveness, Piotr. 🙇

On Thu, Nov 2, 2023 at 6:14 PM Piotr P. Karwasz 
wrote:

> Hi Volkan,
>
> On Thu, 2 Nov 2023 at 16:16, Volkan Yazıcı  wrote:
> > I deleted all date parsing functionality in Log4j[1] and the
> compilation...
> > [drum roll...] succeeded. I am inclined to commit this breaking change.
> We
> > can refer users who were previously using Log4j to parse dates (ugh!) to
> > Commons Lang. The API is identical, they simply need to use a
> > separate package. *Objections?*
>
> I have mixed feelings about that. Technically it is a breaking change
> in the API, since the package was never considered as private.
>
> On the other hand the alternative would be to either to remove the
> `parser` field in `FastDateFormat` and change all the `parse` methods
> to throw an exception or to rewire everything to use `FastDatePrinter`
> instead of `FastDateFormat`.
>
> Piotr
>

Re: [LOG4J2-3672] Time zone parsing in `FastDateParser`

[Piotr P. Karwasz]

Hi Volkan,

On Thu, 2 Nov 2023 at 18:50, Volkan Yazıcı  wrote:
>
> I guess the safest alternative would be to lazily initialize the
> `FastDateFormat#parser` field.

I am happy we agree on the right solution, respectful of the public
API! Now you have my blessing to shred those unused classes to pieces.

Can you also cherry-pick it to `main`?

Piotr

Re: [LOG4J2-3672] Time zone parsing in `FastDateParser`

[Piotr P. Karwasz]

Hi Volkan,

On Thu, 2 Nov 2023 at 16:16, Volkan Yazıcı  wrote:
> I deleted all date parsing functionality in Log4j[1] and the compilation...
> [drum roll...] succeeded. I am inclined to commit this breaking change. We
> can refer users who were previously using Log4j to parse dates (ugh!) to
> Commons Lang. The API is identical, they simply need to use a
> separate package. *Objections?*

I have mixed feelings about that. Technically it is a breaking change
in the API, since the package was never considered as private.

On the other hand the alternative would be to either to remove the
`parser` field in `FastDateFormat` and change all the `parse` methods
to throw an exception or to rewire everything to use `FastDatePrinter`
instead of `FastDateFormat`.

Piotr

Re: [LOG4J2-3672] Time zone parsing in `FastDateParser`

[Ralph Goers]

+1 to this change.

Ralph

> On Nov 2, 2023, at 8:16 AM, Volkan Yazıcı  wrote:
> 
> I deleted all date parsing functionality in Log4j[1] and the compilation...
> [drum roll...] succeeded. I am inclined to commit this breaking change. We
> can refer users who were previously using Log4j to parse dates (ugh!) to
> Commons Lang. The API is identical, they simply need to use a
> separate package. *Objections?*
> 
> [1] To be precise, in `org.apache.logging.log4j.core.util.datetime`
> package, some methods from `FastDateFormat` and `Format`, and
> `FastDateParser[Test]` classes. That is, touched to 4 files in total.
> 

Re: [LOG4J2-3672] Time zone parsing in `FastDateParser`

[Volkan Yazıcı]

I deleted all date parsing functionality in Log4j[1] and the compilation...
[drum roll...] succeeded. I am inclined to commit this breaking change. We
can refer users who were previously using Log4j to parse dates (ugh!) to
Commons Lang. The API is identical, they simply need to use a
separate package. *Objections?*

[1] To be precise, in `org.apache.logging.log4j.core.util.datetime`
package, some methods from `FastDateFormat` and `Format`, and
`FastDateParser[Test]` classes. That is, touched to 4 files in total.

On Thu, Nov 2, 2023 at 10:00 AM Volkan Yazıcı  wrote:

> *Conclusion:* I will make time zone parsing in `FastDateParser` *slow* to
> avoid `getZoneStrings()`.
>
> *Remark:* JTL has the most versatile `Instant` formatter combining
> `FixedDateFormat`, `FastDateFormat`, and `DateTimeFormatter`. See
> `InstantFormatter` for why and how. I will later hoist this up to
> `log4j-core` and make sure every `Instant` formatting is performed via that.
>
>
> On Thu, Nov 2, 2023 at 12:27 AM Piotr P. Karwasz 
> wrote:
>
>> Hi Ralph
>>
>> On Wed, 1 Nov 2023 at 23:53, Ralph Goers 
>> wrote:
>> > > On Nov 1, 2023, at 3:33 PM, Piotr P. Karwasz 
>> wrote:
>> > > Actually we don't need a fast formatter either. Except for the rolling
>> > > file manager that can require a date from the past, the layouts format
>> > > timestamps close to 'now'.
>> > >
>> > > We can format all dates the same way `FixedDateFormat` does: we let
>> > > any date formatter to format the date part once a day and we format
>> > > the time part ourselves.
>> >
>> > Are we doing this now? If not, why not? The only trick is to ensure the
>> date gets formatted on or before the date rollover.
>>
>> Yes and no. If the user specifies one of a finite set of formats in
>> `FixedDateFormat.FixedFormat`, then we only format the date part once
>> a day.
>> If the user chooses another format, then we fall back on the Commons
>> Lang `FastDateFormat`.
>>
>> > One other thing though - don’t we allow different layouts to use
>> different date formats? We would need to ensure every layout is doing this.
>>
>> Different layouts format dates differently. E.g. PatternLayout uses
>> the patterns from the original `SimpleDateFormat`, while
>> JsonTemplateLayout uses the patterns from Java 8 `DateTimeFormatter`.
>> Under the hood they use `FixedDateFormat` if they can. We actually
>> have the unification of datetime formatting as one of our milestones.
>>
>> Piotr
>>
>

Re: [LOG4J2-3672] Time zone parsing in `FastDateParser`

[Volkan Yazıcı]

*Conclusion:* I will make time zone parsing in `FastDateParser` *slow* to
avoid `getZoneStrings()`.

*Remark:* JTL has the most versatile `Instant` formatter combining
`FixedDateFormat`, `FastDateFormat`, and `DateTimeFormatter`. See
`InstantFormatter` for why and how. I will later hoist this up to
`log4j-core` and make sure every `Instant` formatting is performed via that.


On Thu, Nov 2, 2023 at 12:27 AM Piotr P. Karwasz 
wrote:

> Hi Ralph
>
> On Wed, 1 Nov 2023 at 23:53, Ralph Goers 
> wrote:
> > > On Nov 1, 2023, at 3:33 PM, Piotr P. Karwasz 
> wrote:
> > > Actually we don't need a fast formatter either. Except for the rolling
> > > file manager that can require a date from the past, the layouts format
> > > timestamps close to 'now'.
> > >
> > > We can format all dates the same way `FixedDateFormat` does: we let
> > > any date formatter to format the date part once a day and we format
> > > the time part ourselves.
> >
> > Are we doing this now? If not, why not? The only trick is to ensure the
> date gets formatted on or before the date rollover.
>
> Yes and no. If the user specifies one of a finite set of formats in
> `FixedDateFormat.FixedFormat`, then we only format the date part once
> a day.
> If the user chooses another format, then we fall back on the Commons
> Lang `FastDateFormat`.
>
> > One other thing though - don’t we allow different layouts to use
> different date formats? We would need to ensure every layout is doing this.
>
> Different layouts format dates differently. E.g. PatternLayout uses
> the patterns from the original `SimpleDateFormat`, while
> JsonTemplateLayout uses the patterns from Java 8 `DateTimeFormatter`.
> Under the hood they use `FixedDateFormat` if they can. We actually
> have the unification of datetime formatting as one of our milestones.
>
> Piotr
>

Re: [LOG4J2-3672] Time zone parsing in `FastDateParser`

[Piotr P. Karwasz]

Hi Ralph

On Wed, 1 Nov 2023 at 23:53, Ralph Goers  wrote:
> > On Nov 1, 2023, at 3:33 PM, Piotr P. Karwasz  
> > wrote:
> > Actually we don't need a fast formatter either. Except for the rolling
> > file manager that can require a date from the past, the layouts format
> > timestamps close to 'now'.
> >
> > We can format all dates the same way `FixedDateFormat` does: we let
> > any date formatter to format the date part once a day and we format
> > the time part ourselves.
>
> Are we doing this now? If not, why not? The only trick is to ensure the date 
> gets formatted on or before the date rollover.

Yes and no. If the user specifies one of a finite set of formats in
`FixedDateFormat.FixedFormat`, then we only format the date part once
a day.
If the user chooses another format, then we fall back on the Commons
Lang `FastDateFormat`.

> One other thing though - don’t we allow different layouts to use different 
> date formats? We would need to ensure every layout is doing this.

Different layouts format dates differently. E.g. PatternLayout uses
the patterns from the original `SimpleDateFormat`, while
JsonTemplateLayout uses the patterns from Java 8 `DateTimeFormatter`.
Under the hood they use `FixedDateFormat` if they can. We actually
have the unification of datetime formatting as one of our milestones.

Piotr

Re: [LOG4J2-3672] Time zone parsing in `FastDateParser`

[Ralph Goers]

> On Nov 1, 2023, at 3:33 PM, Piotr P. Karwasz  wrote:
> 
> Hi Volkan,
> 
> On Wed, 1 Nov 2023 at 12:07, Volkan Yazıcı  wrote:
>> Curious: *In the context of logging, does `FastDateParser` need to be fast
>> while parsing?* We certainly need to *"format"* the instant fast. Though do
>> we really need to *"parse"* it fast too?
> 
> No we don't. In Commons Lang the parser and formatter  are bound
> together, that is why we have a parser.
> 
> Actually we don't need a fast formatter either. Except for the rolling
> file manager that can require a date from the past, the layouts format
> timestamps close to 'now'.
> 
> We can format all dates the same way `FixedDateFormat` does: we let
> any date formatter to format the date part once a day and we format
> the time part ourselves.

Are we doing this now? If not, why not? The only trick is to ensure the date 
gets formatted on or before the date rollover. 

One other thing though - don’t we allow different layouts to use different date 
formats? We would need to ensure every layout is doing this.

Ralph

Re: [LOG4J2-3672] Time zone parsing in `FastDateParser`

[Piotr P. Karwasz]

Hi Volkan,

On Wed, 1 Nov 2023 at 12:07, Volkan Yazıcı  wrote:
> Curious: *In the context of logging, does `FastDateParser` need to be fast
> while parsing?* We certainly need to *"format"* the instant fast. Though do
> we really need to *"parse"* it fast too?

No we don't. In Commons Lang the parser and formatter  are bound
together, that is why we have a parser.

Actually we don't need a fast formatter either. Except for the rolling
file manager that can require a date from the past, the layouts format
timestamps close to 'now'.

We can format all dates the same way `FixedDateFormat` does: we let
any date formatter to format the date part once a day and we format
the time part ourselves.

Piotr

Re: [LOG4J2-3672] Time zone parsing in `FastDateParser`

[Matt Sicker]

I can’t think of any reason why we’d need fast date parsing. Unless it’s 
relevant in the rolling file appender family?

> On Nov 1, 2023, at 6:07 AM, Volkan Yazıcı  wrote:
> 
> As reported in LOG4J2-3672
> , `FastDateParser`[1]
> contains `DateFormatSymbols#getZoneStrings()`, which causes initialization
> and caching of all time zones, resulting in a ~3MB heap overhead on x86_64.
> The reporter also provided the PR #1848
> , though it needs
> several assumptions to hold to be effective.
> 
> Curious: *In the context of logging, does `FastDateParser` need to be fast
> while parsing?* We certainly need to *"format"* the instant fast. Though do
> we really need to *"parse"* it fast too?
> 
> [1] FDP was borrowed from Apache Commons Lang in 2015. I have checked the
> most recent version
> ,
> they still use `getZoneStrings()`.

[LOG4J2-3672] Time zone parsing in `FastDateParser`

[Volkan Yazıcı]

As reported in LOG4J2-3672
, `FastDateParser`[1]
contains `DateFormatSymbols#getZoneStrings()`, which causes initialization
and caching of all time zones, resulting in a ~3MB heap overhead on x86_64.
The reporter also provided the PR #1848
, though it needs
several assumptions to hold to be effective.

Curious: *In the context of logging, does `FastDateParser` need to be fast
while parsing?* We certainly need to *"format"* the instant fast. Though do
we really need to *"parse"* it fast too?

[1] FDP was borrowed from Apache Commons Lang in 2015. I have checked the
most recent version
,
they still use `getZoneStrings()`.