cleanups of DateCache

[lachlan-roberts]

• Fix incorrect and misleading javadoc.
• DateCache.format now actually uses the cache.
• Introduce formatWithoutCache method.
• Deprecate unnecessary methods.
• Formatting code in constructor for ZZZ is unnecessary.

Maybe this should be for 12 only, there could be some change in behaviour if someone is relying on DateCache.format to not use the cache (even though the javadoc says it does).

[gregw]

I think you've missed the difference between format and formatNow. With this change, the cache is now vulnerable to being set to a tick from any time and thus could miss more frequently.

Perhaps we can just have a single format method, but it must only update the _tick with the current time.

[gregw]

This is now being used to format arbitrary times, so the tick cache could be hoping all over time.

[gregw]

Maybe we change this to never cache a passed in time, but to only ever cache the format of now. If the passed in time is in a different tick, we then check the tick time against now and only reformat that if now has changed?

[sbordet]

@gregw the major user of DateCache is CustomRequestLog (there are usages in DebugHandler and similar, but they are minor).

For the minor usages, I think we can just format on-the-fly all the time.

In CustomRequestLog, we use the request time (from the request object, not the current time).
For this case, I think the cache has a reason to be for cases where we have 1000 requests/s to be logged, and likely many of them have a request time within the same second, although depending on the traffic that might also not be the case.

The other use case, currently unused in the Jetty codebase, is for applications that want to set the Date header efficiently.
This use case is slightly different from the CustomRequestLog use case (since the time is always the current time).

I'm not sure DateCache can solve both -- feels like CustomRequestLog needs a different mechanism, where perhaps the last N seconds are cached.
If we use a different solution for CustomRequestLog, then do we retain DateCache for applications only, since Jetty does not use it internally?

[joakime]

I'm not sure DateCache can solve both -- feels like CustomRequestLog needs a different mechanism, where perhaps the last N seconds are cached.

If someone wants to format their request timestamp with millisecond accuracy (see #10153), how will a DateCache handle that?

[sbordet]

If someone wants to format their request timestamp with millisecond accuracy (see #10153), how will a DateCache handle that?

It cannot.
We would need to use an on-the-fly formatting, with a new % specifier that we currently don't have.

[gregw]

@joakime

If someone wants to format their request timestamp with millisecond accuracy (see #10153), how will a DateCache handle that?

The DateCache used to have a mechanism where it separately handled milliseconds by adding them them to the pre-formatted string for the second. I can't see that code anymore, not sure when it was deleted. But it definitely was possible and was definitely faster than reformatting the entire date. The class was primarily used for the Date header that only has seconds resolution, so perhaps that got dropped. I can definitely see the need for milliseconds in the request log, so perhaps we want to consider re-implementing that feature?

[gregw]

I'm not sure DateCache can solve both -- feels like CustomRequestLog needs a different mechanism, where perhaps the last N seconds are cached.

Maybe... also with millisecond support again would be good.

I think just having the last 2 seconds should be sufficient, as if requests are taking more than 2s to complete, then formatting dates is not going to be the hot path (OK perhaps if they are really async?)

The main issue will be for quick requests that are finishing in about the same time, then as the second clicks from 1 to the next, we might get the requests from different threads in the wrong order, thus a lot more formatting might take place as the second changes over.

The simplest fix would be to only let the tick monotonically increase and never go back in time, whilst always formatting the current time. That would reduce the cache itself fibrillating as the second changes, but it might mean that a few threads will finish requests for the previous second and need to do formatting. So having the current second and the previous second would probably save 10s of formats per second, not a huge saving for a bit of extra complexity.

Eitherway, I agree that we leave the server using it's Server.getDateField() method as that is simpler and more frequently used. Then DateCache can be updated to be ideal for the the request log case (however I think it should remain as a util class).

[gregw]

It cannot.
We would need to use an on-the-fly formatting, with a new % specifier that we currently don't have.

@sbordet
Once upon a time we supported it (but before git apparently). We just looked for the 'SSS' pattern and split the formatted string there (or did a format for before and after (or maybe we only supported trailing SSS)), then used a format of "%s%03d%s" to join the prefix/suffix with the milliseconds within the tick. It is a bit of faff, but still faster than a general format.

[joakime]

It cannot.
We would need to use an on-the-fly formatting, with a new % specifier that we currently don't have.

@sbordet Once upon a time we supported it (but before git apparently). We just looked for the 'SSS' pattern and split the formatted string there (or did a format for before and after (or maybe we only supported trailing SSS)), then used a format of "%s%03d%s" to join the prefix/suffix with the milliseconds within the tick. It is a bit of faff, but still faster than a general format.

Looking back at Jetty 6, we didn't support SSS then too.
In fact, we even produced an exception if SSS was present.

https://github.com/jetty-project/codehaus-jetty6/blob/jetty-6.1/modules/util/src/main/java/org/mortbay/util/DateCache.java#L193-L196

[joakime]

Ah, Jetty 6.0.2 had support for SSS ...

https://github.com/jetty-project/codehaus-jetty6/blob/jetty-6.0.2/modules/util/src/main/java/org/mortbay/util/DateCache.java#L192-L209

So the support for SSS was removed in commit jetty-project/codehaus-jetty6@0febdc9 (back in May 22, 2007, for the Jetty 6.1.4 release)

[gregw]

Ah, Jetty 6.0.2 had support for SSS ...

phew!!! the memory is not totally shot!.... yet!

[lachlan-roberts]

@gregw an index in the format string does not correspond to the same index in the formatted date, many of the date codes expand to different sizes when formatting a date, even dynamically depending on the time of day / week / month etc. So we can only figure out the correct index when we create the Tick.

Using separate formatters for the suffix and prefix is marginally faster, but both are around 10 times slower than using the cached ms time.

[gregw]

close but....

[sbordet]

Fairly complicated now, but speed(ier).

JMH results on my hardware, compensating for the Instant allocation so that testDateCache() does Instant.now().toEpochMillis():

DateCacheBenchmark.testDateCache          thrpt    3  42247477.161 ± 51521167.555  ops/s
DateCacheBenchmark.testDateTimeFormatter  thrpt    3   8058916.049 ±   466843.119  ops/s

[gregw]

Just niggles... but let's get this merged

[gregw]

Suggested change

	public String getString(long inDate)
	public String format(long inDate)

[lachlan-roberts]

done

[gregw]

Could this be a record?

[lachlan-roberts]

no, this is jetty-10 so we can't use records

[gregw]

it was 2 lines you changed not 1 :)

[joakime]

@lachlan-roberts can you squash merge this soon?

[gregw]

@lachlan-roberts do you need superpowers to merge this to 11 & 12 or are you going to do PRs?

[lachlan-roberts]

@gregw I can merge to 11, but will need to do PR to 12. But @sbordet is the one who merged the PR so I assume he did it, but I will double check.

There were outstanding commits from 11->12 so I was waiting until that was resolved before merging this.

[lachlan-roberts]

Yes, it has been merged to 12 already.