Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow to set thread-local calendars and calendar prototype for RegularTimePeriod #171

Merged
merged 2 commits into from
Nov 15, 2020

Conversation

stachenov
Copy link
Contributor

In JFreeChart 1.5.0, RegularTimePeriod subclasses create a new calendar instance every time they need one, which is during every construction, including via next() and previous() methods. In most setups, this creates a new GregorianCalendar instance, which is very heavyweight (at least two arrays of int and one array of boolean, 17 elements each). For example, when creating and displaying a chart for 86400 seconds (one day's worth), this doubles memory consumption (200–250 MB instead of slightly above 100). The GC simply can't cope fast enough with this.

Since the calendars are only used temporarily for time calculations (“pegging”) and then immediately discarded, it would make sense to use a single global instance. However, since Calendar is mutable, it would create concurrency problems with multi-threaded applications. It's not a problem for displaying a single chart with Swing, but could be a problem for an application that performs some kind of batch processing, producing a lot of charts for a lot of input files. Therefore, it makes sense to make those calendars thread-local.

This PR introduces three methods:

  • RegularTimePeriod.setThreadLocalCalendarInstance()—a public method that allows every thread to set a thread-local calendar for every future RegularTimePeriod creation (except with constructors that accept a time zone or a calendar explicitly).
  • RegularTimePeriod.setCalendarInstancePrototype()—a public method that allows to set a global prototype (as in the Prototype Pattern) instance that will be then cloned by every thread as needed and cached locally as if it was set with setThreadLocalCalendarInstance().
    -RegularTimePeriod.getCalendarInstance()—a protected method that is now used by all RegularTimePeriod subclasses where Calendar.getInstance() was used previously. If none of the set*() methods are called, this method resorts to JFreeChart 1.5.0 behavior, spamming a new instance every time one is needed, which means users that don't call the new set*() methods won't be affected in any way.

Constructors accepting a Calendar explicitly were added as well. This lead to some code duplication between (..., TimeZone, Locale) and the new (..., Calendar) constructors. It can be fixed by either generifying Args.nullNotPermitted() to return the first argument (much like Objects.requireNonNull() does), or by switching to Objects.requireNonNull() (which would change the exception type from IllegalArgumentException to NullPointerException). Please let me know if that's desirable, so I can add another commit to this PR.

@jfree
Copy link
Owner

jfree commented Oct 31, 2020

I like this approach, but haven't had time to dig deeper into it yet.

A thread-local calendar can be set with
RegularTimePeriod.setThreadLocalCalendarInstance() to use for pegging
various RegularTimePeriod instances, avoiding the need to create a new
calendar instance every time one is needed (this creates huge load on GC
and high memory consumption in scenarios where there are many instances).

To avoid setting an instance for every thread, a global prototype can be
set with RegularTimePeriod.setCalendarInstancePrototype(), which then
will be automatically cloned and cached by every thread.

Calendar instances are now obtained with
RegularTimePeriod.getCalendarInstance(), which resorts to spamming new
default calendar instances every time (old behavior) if neither
a thread-local instance nor a prototype were set.
@stachenov
Copy link
Contributor Author

I've rebased it on top of the current master to get rid of the conflicts, so that it's easier to review the changes.

@jfree jfree merged commit 0806238 into jfree:master Nov 15, 2020
@jfree
Copy link
Owner

jfree commented Nov 15, 2020

Great work, thanks! I've merged it.

I didn't fully understand your last point about generifying the Args.nullNotPermitted() method - how does it help?

@stachenov stachenov deleted the default-calendar branch November 15, 2020 07:17
@stachenov
Copy link
Contributor Author

stachenov commented Nov 15, 2020

I don't remember the exact places, but the idea is that when you delegate one constructor to another, you can't write:

MyClass(String arg1, String arg2) {
    Args.nullNotPermitted(arg1);
    this(arg1, "");
}

because this has to be the first call. So I had to duplicate code in a few places, where delegation would work better.

But if Args.nullNotPermitted() has a signature like

public <T> T nullNotPermitted(T value) {
    if (value == null)
        throw ...
    return value;
}

then you can do

MyClass(String arg1, String arg2) {
    this(Args.nullNotPermitted(arg1), "");
}

This is now OK, because this is the first call (actual argument expression don't count if they don't reference this, neither explicitly nor implicitly).

...or simply replace nullNotPermitted() with the standard Objects.requireNonNull() which works in exactly the same way, but it throws a different kind of exception, which is sort of a backwards-incompatible change.

@jfree
Copy link
Owner

jfree commented Nov 15, 2020

Ah, nice! I'll give that a try.

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.

2 participants