Re: TimeZone + Calendar
Thanks, done Tilman On 12.08.2023 15:19, Daniel Gredler wrote: Hi Tilman, Thanks! I would have sent a patch, but I've run into some build issues :-) I think cloning would just ensure that the static final `TimeZone` instance is not modified by client code (i.e. defensive copy). For example, if an application is using the `Calendar` instance provided by `readInternationalDate()`, and calls `calendar.getTimeZone().setRawOffset(foo)`, I think they will have modified the offset of the UTC constant. Not very common, but a possible source of errors. I think there are only two methods which might modify the TimeZone, so it's *almost* immutable, but not quite. The `TimeZone` internals are funky, but it seems like depending on how the `TimeZone` is created, it also sometimes needs to create a defensive copy in order to stay safe. @Andreas: Thanks for the information about Calendar vs `java.time` APIs. It's not a biggie, but the new APIs do have some advantages (for this situation: fully immutable time zones, and an out-of-the-box UTC constant). Daniel On Thu, Aug 10, 2023 at 6:38 AM Tilman Hausherr wrote: On 08.08.2023 19:30, Daniel Gredler wrote: If not, what about keeping a static final UTC `TimeZone` constant and creating a clone each time it is needed, to avoid the synchronization? (`Calendar.getTimeZone()` makes liberal use of `clone()`, for example). I've added a static final for UTC but no cloning yet; why would we have to clone it when needed? Why does java do it? I looked a bit in the source code, there is a "sharedZone" flag, but it's unclear if set or not. https://issues.apache.org/jira/browse/PDFBOX-5645 Tilman - To unsubscribe, e-mail: dev-unsubscr...@pdfbox.apache.org For additional commands, e-mail: dev-h...@pdfbox.apache.org - To unsubscribe, e-mail: dev-unsubscr...@pdfbox.apache.org For additional commands, e-mail: dev-h...@pdfbox.apache.org
Re: TimeZone + Calendar
Hi Tilman, Thanks! I would have sent a patch, but I've run into some build issues :-) I think cloning would just ensure that the static final `TimeZone` instance is not modified by client code (i.e. defensive copy). For example, if an application is using the `Calendar` instance provided by `readInternationalDate()`, and calls `calendar.getTimeZone().setRawOffset(foo)`, I think they will have modified the offset of the UTC constant. Not very common, but a possible source of errors. I think there are only two methods which might modify the TimeZone, so it's *almost* immutable, but not quite. The `TimeZone` internals are funky, but it seems like depending on how the `TimeZone` is created, it also sometimes needs to create a defensive copy in order to stay safe. @Andreas: Thanks for the information about Calendar vs `java.time` APIs. It's not a biggie, but the new APIs do have some advantages (for this situation: fully immutable time zones, and an out-of-the-box UTC constant). Daniel On Thu, Aug 10, 2023 at 6:38 AM Tilman Hausherr wrote: > On 08.08.2023 19:30, Daniel Gredler wrote: > > If not, what about keeping a static final UTC `TimeZone` constant and > > creating a clone each time it is needed, to avoid the synchronization? > > (`Calendar.getTimeZone()` makes liberal use of `clone()`, for example). > > I've added a static final for UTC but no cloning yet; why would we have > to clone it when needed? Why does java do it? > > I looked a bit in the source code, there is a "sharedZone" flag, but > it's unclear if set or not. > > https://issues.apache.org/jira/browse/PDFBOX-5645 > > Tilman > > > > - > To unsubscribe, e-mail: dev-unsubscr...@pdfbox.apache.org > For additional commands, e-mail: dev-h...@pdfbox.apache.org > >
Re: TimeZone + Calendar
On 08.08.2023 19:30, Daniel Gredler wrote: If not, what about keeping a static final UTC `TimeZone` constant and creating a clone each time it is needed, to avoid the synchronization? (`Calendar.getTimeZone()` makes liberal use of `clone()`, for example). I've added a static final for UTC but no cloning yet; why would we have to clone it when needed? Why does java do it? I looked a bit in the source code, there is a "sharedZone" flag, but it's unclear if set or not. https://issues.apache.org/jira/browse/PDFBOX-5645 Tilman - To unsubscribe, e-mail: dev-unsubscr...@pdfbox.apache.org For additional commands, e-mail: dev-h...@pdfbox.apache.org
Re: TimeZone + Calendar
Hi, see inline Am 08.08.23 um 19:30 schrieb Daniel Gredler: Hi, I think I've said this before, but thanks again for such a great library! I'm thinking about submitting a patch to improve PDFBox, but wanted to get the team's thoughts first. What I've seen is that sometimes under very heavy load across multiple threads, PDF creation stalls due to lock contention, because `TTFDataStream.readInternationalDate()` uses `TimeZone.getTimeZone(String)`, which is synchronized. I assume that the inverse operation, `TTFSubsetter.writeLongDateTime(DataOutputStream,Calendar)`, has a similar issue, though I have not seen it myself. Good catch. PDFBox is not supposed to be thread-safe, but it is always a good idea to eliminate obvious issues. If these classes were using the newer `java.time` APIs, they could simply use the `ZoneOffset.UTC` constant... but they are using the older `Calendar` API (and exposing this fact), so `TimeZone` must be used. So a few questions from my side: Is there a plan to move off of the older time APIs and onto the newer `java.time` APIs, perhaps as part of version 3.0? (e.g. `Calendar` -> `ZonedDateTime`) Either way, I'm pretty sure such a breaking change is not contemplated for the 2.x release series, correct? No plan so far. There were some discussions in the past but other things very more important or more interesting ;-) There were some small changes in the trunk. We already released a beta of 3.0 which implies a stable api so that we must not change that. Furthermore I'm planing to cut the final release next Monday and I guess there isn't enough time to do such changes at all. 2.x is a no go as well, as it relies on java 6 and the java.time api requires java 8 `TimeZone` is not technically thread-safe, but there only a couple of rarely-used dangerous mutators, AFAIK (`setRawOffset(int)`, `setID(String)`). Would it be OK to keep a static final UTC `TimeZone` and just reuse that, even though `TimeZone` is only 95% immutable? I concur with Tilman, this would be a small but good solution for 3.0 and 2.x as well Let's discuss the topic after releasing 3.0. Maybe it is a good idea to target 4.x for such a change. If not, what about keeping a static final UTC `TimeZone` constant and creating a clone each time it is needed, to avoid the synchronization? (`Calendar.getTimeZone()` makes liberal use of `clone()`, for example). If that doesn't work, what about subclassing `TimeZone` and overriding the mutators with methods that just throw `UnsupportedOperationException`s, and using the subclass for the static final UTC constant? Commons Lang 3 does something similar with `GmtTimeZone` and the `FastTimeZone` utility class here: https://github.com/apache/commons-lang/blob/bf5865ae915ececcdbfa7a473b0d708e3e235bcf/src/main/java/org/apache/commons/lang3/time/FastTimeZone.java#L38 Thanks for the feedback! Daniel - To unsubscribe, e-mail: dev-unsubscr...@pdfbox.apache.org For additional commands, e-mail: dev-h...@pdfbox.apache.org
Re: TimeZone + Calendar
Hi,
So many alternnatives...
But making TimeZone.getTimeZone("UTC") a static final is definitively a
good idea and could be done for 2.* too.
Tilman
On 08.08.2023 19:30, Daniel Gredler wrote:
Hi,
I think I've said this before, but thanks again for such a great library!
I'm thinking about submitting a patch to improve PDFBox, but wanted to get
the team's thoughts first.
What I've seen is that sometimes under very heavy load across multiple
threads, PDF creation stalls due to lock contention, because
`TTFDataStream.readInternationalDate()` uses
`TimeZone.getTimeZone(String)`, which is synchronized. I assume that the
inverse operation,
`TTFSubsetter.writeLongDateTime(DataOutputStream,Calendar)`, has a similar
issue, though I have not seen it myself.
If these classes were using the newer `java.time` APIs, they could simply
use the `ZoneOffset.UTC` constant... but they are using the older
`Calendar` API (and exposing this fact), so `TimeZone` must be used. So a
few questions from my side:
Is there a plan to move off of the older time APIs and onto the newer
`java.time` APIs, perhaps as part of version 3.0? (e.g. `Calendar` ->
`ZonedDateTime`) Either way, I'm pretty sure such a breaking change is not
contemplated for the 2.x release series, correct?
`TimeZone` is not technically thread-safe, but there only a couple of
rarely-used dangerous mutators, AFAIK (`setRawOffset(int)`,
`setID(String)`). Would it be OK to keep a static final UTC `TimeZone` and
just reuse that, even though `TimeZone` is only 95% immutable?
If not, what about keeping a static final UTC `TimeZone` constant and
creating a clone each time it is needed, to avoid the synchronization?
(`Calendar.getTimeZone()` makes liberal use of `clone()`, for example).
If that doesn't work, what about subclassing `TimeZone` and overriding the
mutators with methods that just throw `UnsupportedOperationException`s, and
using the subclass for the static final UTC constant? Commons Lang 3 does
something similar with `GmtTimeZone` and the `FastTimeZone` utility class
here:
https://github.com/apache/commons-lang/blob/bf5865ae915ececcdbfa7a473b0d708e3e235bcf/src/main/java/org/apache/commons/lang3/time/FastTimeZone.java#L38
Thanks for the feedback!
Daniel
-
To unsubscribe, e-mail: dev-unsubscr...@pdfbox.apache.org
For additional commands, e-mail: dev-h...@pdfbox.apache.org
