You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mina.apache.org by "chuckbinder (via GitHub)" <gi...@apache.org> on 2025/04/26 15:23:21 UTC
[PR] Reduce GC Pressure by Storing Last Access Time as Primitive Long [mina-ftpserver]
chuckbinder opened a new pull request, #76:
URL: https://github.com/apache/mina-ftpserver/pull/76
This change optimizes FtpIoSession by internally storing lastAccessTime as a long instead of a Date.
During file transfers, lastAccessTime is updated every 4 KB, leading to frequent Date object allocations.
In contrast, reads of lastAccessTime occur infrequently — typically only after FTP command execution, during idle timeout checks, or via rare administrative commands.
By using a long and creating a Date only when accessed, we eliminate unnecessary object churn, significantly reducing GC overhead and memory fragmentation in high-concurrency environments.
This improves throughput and scalability without affecting API compatibility.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
Re: [PR] Reduce GC Pressure by Storing Last Access Time as Primitive Long [mina-ftpserver]
Posted by "chuckbinder (via GitHub)" <gi...@apache.org>.
chuckbinder commented on PR #76:
URL: https://github.com/apache/mina-ftpserver/pull/76#issuecomment-2832486444
Thank you for pointing out the boxing concern!
Using a scenario of 100 clients connecting in and transferring 1 GB File concurrently the following is saved:
Each transfer triggers 262,144 lastAccessTime updates.
With the current design, this results in 26 million temporary Date object allocations.
Switching to storing System.currentTimeMillis() (boxed Long) reduces object size but still creates 26 million Long allocations.
Introducing LazyDateHolder eliminates per-update allocations completely by maintaining a single reusable object with a primitive long, creating a Date only if necessary.
This significantly reduces GC pressure and improves server scalability under load.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
Re: [PR] Reduce GC Pressure by Storing Last Access Time as Primitive Long [mina-ftpserver]
Posted by "tomaswolf (via GitHub)" <gi...@apache.org>.
tomaswolf commented on code in PR #76:
URL: https://github.com/apache/mina-ftpserver/pull/76#discussion_r2061413053
##########
core/src/main/java/org/apache/ftpserver/impl/FtpIoSession.java:
##########
@@ -924,7 +924,7 @@ public Certificate[] getClientCertificates() {
* Update the last-access-time session attribute with the current date
*/
public void updateLastAccessTime() {
- setAttribute(ATTRIBUTE_LAST_ACCESS_TIME, new Date());
+ setAttribute(ATTRIBUTE_LAST_ACCESS_TIME, System.currentTimeMillis());
Review Comment:
Given that long is auto-boxed to Long here I don't really see how this would reduce GC churn.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
Re: [PR] Reduce GC Pressure by Storing Last Access Time as Primitive Long [mina-ftpserver]
Posted by "tomaswolf (via GitHub)" <gi...@apache.org>.
tomaswolf commented on PR #76:
URL: https://github.com/apache/mina-ftpserver/pull/76#issuecomment-2832634594
> creating a Date only if necessary.
That's not what your code does. Besides, you've now introduced two synchronized methods. I think the approach is right, but this code is not. Try something like
```
public class DateHolder {
private volatile long millis;
public DateHolder() {
this(System.currentTimeMillis());
}
public DateHolder(long initial) {
millis = initial;
}
public update() {
millis = System.currentTimeMillis();
}
public getDate() {
return new Date(millis);
}
}
```
That's all you'd need.
How about storing an AtomicLong in that attribute and just update that? Then you don't even need a custom class.
Please try to avoid unrelated reformatting, it makes review difficult.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
Re: [PR] Reduce GC Pressure by Storing Last Access Time as Primitive Long [mina-ftpserver]
Posted by "chuckbinder (via GitHub)" <gi...@apache.org>.
chuckbinder closed pull request #76: Reduce GC Pressure by Storing Last Access Time as Primitive Long
URL: https://github.com/apache/mina-ftpserver/pull/76
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
Re: [PR] Reduce GC Pressure by Storing Last Access Time as Primitive Long [mina-ftpserver]
Posted by "tomaswolf (via GitHub)" <gi...@apache.org>.
tomaswolf commented on code in PR #76:
URL: https://github.com/apache/mina-ftpserver/pull/76#discussion_r2062561624
##########
core/src/main/java/org/apache/ftpserver/util/LazyDateHolder.java:
##########
@@ -0,0 +1,98 @@
+package org.apache.ftpserver.util;
+
+import java.util.Date;
+
+/**
+ * A thread-safe holder for a timestamp that lazily creates a corresponding {@link Date} object.
+ * <p>
+ * This class is designed to optimize scenarios where the timestamp is frequently updated
+ * (such as during file transfers), but the {@code Date} object is rarely accessed.
+ * A new {@code Date} instance is created only when the timestamp value changes,
+ * minimizing unnecessary object allocations and reducing garbage collection pressure.
+ * </p>
+ *
+ * <p>
+ * All methods that access or modify the cached {@code Date} are properly synchronized
+ * to ensure atomic visibility of the timestamp and associated {@code Date}.
+ * </p>
+ */
+public final class LazyDateHolder {
+
+ private long timeMillis;
+
+ private Date cachedDate;
+
+ /**
+ * Creates a new {@code LazyDateHolder} initialized with the specified time in milliseconds.
+ *
+ * @param initialTimeMillis
+ * the initial timestamp value in milliseconds since the epoch
+ */
+ public LazyDateHolder(long initialTimeMillis) {
+ this.timeMillis = initialTimeMillis;
+ this.cachedDate = null;
+ }
+
+ /**
+ * Creates a new {@code LazyDateHolder} initialized with the current system time.
+ */
+ public LazyDateHolder() {
+ this(System.currentTimeMillis());
+ }
+
+ /**
+ * Updates the timestamp to the specified value in milliseconds since the epoch.
+ * <p>
+ * If the new timestamp differs from the current one, the cached {@code Date} object is refreshed.
+ * </p>
+ *
+ * @param newTimeMillis
+ * the new timestamp value in milliseconds
+ */
+ public synchronized void update(long newTimeMillis) {
+ if (this.timeMillis != newTimeMillis) {
+ this.timeMillis = newTimeMillis;
+ this.cachedDate = null;
+ }
+ }
+
+ /**
+ * Updates the timestamp to the current system time.
+ * <p>
+ * If the new timestamp differs from the current one, the cached {@code Date} object is refreshed.
+ * </p>
+ */
+ public void update() {
+ update(System.currentTimeMillis());
+ }
+
+ /**
+ * Returns the cached {@code Date} instance representing the current timestamp.
+ * <p>
+ * This {@code Date} object is updated lazily and only changes when the timestamp value changes.
+ * </p>
+ *
+ * @return the cached {@code Date} corresponding to the last update time
+ */
+ public synchronized Date getDate() {
+ if(cachedDate == null)
+ {
+ cachedDate = new Date(timeMillis);
+ }
+ return cachedDate;
+ }
+
+ synchronized boolean isDateNull()
Review Comment:
Please add a comment to clarify that this exists only for testing. Something like
```
// Package-visible for tests
```
##########
core/src/test/java/org/apache/ftpserver/util/LazyDateHolderTest.java:
##########
@@ -0,0 +1,130 @@
+package org.apache.ftpserver.util;
+
+import junit.framework.TestCase;
+
+import java.util.Date;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.atomic.AtomicReference;
+
+/**
+ * Unit tests for {@link LazyDateHolder}.
+ */
+public class LazyDateHolderTest extends TestCase {
+
+ public void testInitialTimeFromConstructor() {
+ long now = System.currentTimeMillis();
+ LazyDateHolder holder = new LazyDateHolder(now);
+ assertEquals("Initial timeMillis should match provided value", now, holder.getTimeMillis());
+ assertEquals("Initial Date should match provided value", now, holder.getDate().getTime());
+ }
+
+ public void testInitialTimeFromNoArgConstructor() {
+ LazyDateHolder holder = new LazyDateHolder();
+ long now = System.currentTimeMillis();
+ long diff = Math.abs(holder.getTimeMillis() - now);
+ assertTrue("Initial timeMillis should be close to current time", diff < 1000);
+ }
+
+ public void testUpdateWithSameTimeDoesNotChangeDate() {
+ long now = System.currentTimeMillis();
+ LazyDateHolder holder = new LazyDateHolder(now);
+ Date firstDate = holder.getDate();
+
+ holder.update(now); // update with same millis
+ Date secondDate = holder.getDate();
+
+ assertSame("Date instance should not change if timeMillis is the same", firstDate, secondDate);
+ }
+
+ public void testUpdateWithDifferentTimeChangesDate() {
+ long now = System.currentTimeMillis();
+ LazyDateHolder holder = new LazyDateHolder(now);
+ Date firstDate = holder.getDate();
+
+ holder.update(now + 1000); // advance by 1 second
+ Date secondDate = holder.getDate();
+
+ assertNotSame("Date instance should change when timeMillis changes", firstDate, secondDate);
+ assertEquals("Date should reflect updated timeMillis", now + 1000, secondDate.getTime());
+ }
+
+ public void testUpdateWithoutArgumentUsesCurrentTime() throws InterruptedException {
+ LazyDateHolder holder = new LazyDateHolder();
+ Date firstDate = holder.getDate();
+ Thread.sleep(5); // ensure time passes
+ holder.update(); // auto update
+ Date secondDate = holder.getDate();
+
+ assertTrue("Date after auto update should be newer", secondDate.getTime() > firstDate.getTime());
+ }
+
+ public void testNullDateWithUpdate() throws InterruptedException {
+ LazyDateHolder holder = new LazyDateHolder();
+ assertTrue("Initially should be null",holder.isDateNull());
+
+ Thread.sleep(5); // ensure time passes
+ holder.update(); // auto update
+ Date secondDate = holder.getDate();
+ assertFalse("Should not be null as date retrieved",holder.isDateNull());
+ Thread.sleep(5); // ensure time passes
+ holder.update(); // auto update
+ assertTrue("Should be not null due to update",holder.isDateNull());
Review Comment:
Comment is wrong; and use `assertNull`.
##########
core/src/main/java/org/apache/ftpserver/util/LazyDateHolder.java:
##########
@@ -0,0 +1,98 @@
+package org.apache.ftpserver.util;
Review Comment:
Missing license header.
##########
core/src/test/java/org/apache/ftpserver/util/LazyDateHolderTest.java:
##########
@@ -0,0 +1,130 @@
+package org.apache.ftpserver.util;
+
+import junit.framework.TestCase;
+
+import java.util.Date;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.atomic.AtomicReference;
+
+/**
+ * Unit tests for {@link LazyDateHolder}.
+ */
+public class LazyDateHolderTest extends TestCase {
+
+ public void testInitialTimeFromConstructor() {
+ long now = System.currentTimeMillis();
+ LazyDateHolder holder = new LazyDateHolder(now);
+ assertEquals("Initial timeMillis should match provided value", now, holder.getTimeMillis());
+ assertEquals("Initial Date should match provided value", now, holder.getDate().getTime());
+ }
+
+ public void testInitialTimeFromNoArgConstructor() {
+ LazyDateHolder holder = new LazyDateHolder();
+ long now = System.currentTimeMillis();
+ long diff = Math.abs(holder.getTimeMillis() - now);
+ assertTrue("Initial timeMillis should be close to current time", diff < 1000);
+ }
+
+ public void testUpdateWithSameTimeDoesNotChangeDate() {
+ long now = System.currentTimeMillis();
+ LazyDateHolder holder = new LazyDateHolder(now);
+ Date firstDate = holder.getDate();
+
+ holder.update(now); // update with same millis
+ Date secondDate = holder.getDate();
+
+ assertSame("Date instance should not change if timeMillis is the same", firstDate, secondDate);
+ }
+
+ public void testUpdateWithDifferentTimeChangesDate() {
+ long now = System.currentTimeMillis();
+ LazyDateHolder holder = new LazyDateHolder(now);
+ Date firstDate = holder.getDate();
+
+ holder.update(now + 1000); // advance by 1 second
+ Date secondDate = holder.getDate();
+
+ assertNotSame("Date instance should change when timeMillis changes", firstDate, secondDate);
+ assertEquals("Date should reflect updated timeMillis", now + 1000, secondDate.getTime());
+ }
+
+ public void testUpdateWithoutArgumentUsesCurrentTime() throws InterruptedException {
+ LazyDateHolder holder = new LazyDateHolder();
+ Date firstDate = holder.getDate();
+ Thread.sleep(5); // ensure time passes
+ holder.update(); // auto update
+ Date secondDate = holder.getDate();
+
+ assertTrue("Date after auto update should be newer", secondDate.getTime() > firstDate.getTime());
+ }
+
+ public void testNullDateWithUpdate() throws InterruptedException {
+ LazyDateHolder holder = new LazyDateHolder();
+ assertTrue("Initially should be null",holder.isDateNull());
+
+ Thread.sleep(5); // ensure time passes
+ holder.update(); // auto update
+ Date secondDate = holder.getDate();
Review Comment:
Unused variable.
##########
core/src/main/java/org/apache/ftpserver/util/LazyDateHolder.java:
##########
@@ -0,0 +1,98 @@
+package org.apache.ftpserver.util;
+
+import java.util.Date;
+
+/**
+ * A thread-safe holder for a timestamp that lazily creates a corresponding {@link Date} object.
+ * <p>
+ * This class is designed to optimize scenarios where the timestamp is frequently updated
+ * (such as during file transfers), but the {@code Date} object is rarely accessed.
+ * A new {@code Date} instance is created only when the timestamp value changes,
+ * minimizing unnecessary object allocations and reducing garbage collection pressure.
+ * </p>
+ *
+ * <p>
+ * All methods that access or modify the cached {@code Date} are properly synchronized
+ * to ensure atomic visibility of the timestamp and associated {@code Date}.
+ * </p>
+ */
+public final class LazyDateHolder {
+
+ private long timeMillis;
+
+ private Date cachedDate;
+
+ /**
+ * Creates a new {@code LazyDateHolder} initialized with the specified time in milliseconds.
+ *
+ * @param initialTimeMillis
+ * the initial timestamp value in milliseconds since the epoch
+ */
+ public LazyDateHolder(long initialTimeMillis) {
+ this.timeMillis = initialTimeMillis;
+ this.cachedDate = null;
+ }
+
+ /**
+ * Creates a new {@code LazyDateHolder} initialized with the current system time.
+ */
+ public LazyDateHolder() {
+ this(System.currentTimeMillis());
+ }
+
+ /**
+ * Updates the timestamp to the specified value in milliseconds since the epoch.
+ * <p>
+ * If the new timestamp differs from the current one, the cached {@code Date} object is refreshed.
+ * </p>
+ *
+ * @param newTimeMillis
+ * the new timestamp value in milliseconds
+ */
+ public synchronized void update(long newTimeMillis) {
+ if (this.timeMillis != newTimeMillis) {
+ this.timeMillis = newTimeMillis;
+ this.cachedDate = null;
+ }
+ }
+
+ /**
+ * Updates the timestamp to the current system time.
+ * <p>
+ * If the new timestamp differs from the current one, the cached {@code Date} object is refreshed.
+ * </p>
+ */
+ public void update() {
+ update(System.currentTimeMillis());
+ }
+
+ /**
+ * Returns the cached {@code Date} instance representing the current timestamp.
+ * <p>
+ * This {@code Date} object is updated lazily and only changes when the timestamp value changes.
+ * </p>
+ *
+ * @return the cached {@code Date} corresponding to the last update time
+ */
+ public synchronized Date getDate() {
+ if(cachedDate == null)
+ {
+ cachedDate = new Date(timeMillis);
+ }
+ return cachedDate;
+ }
+
+ synchronized boolean isDateNull()
+ {
+ return cachedDate == null;
+ }
+
+ /**
+ * Returns the current timestamp value in milliseconds since the epoch.
+ *
+ * @return the current timestamp in milliseconds
+ */
+ public long getTimeMillis() {
+ return timeMillis;
Review Comment:
Unsynchronized access to timeMillis.
If you want this to be thread-safe, how about this:
```
private volatile long timeMillis;
private volatile Date cachedDate;
public void update() {
timeMillis = System.currentTimeMillis();
}
public Date getDate() {
Date cached = cachedDate;
long current = timeMillis;
if (cached == null || cached.getTime() != current) {
cached = new Date(current);
cachedDate = cached;
}
return cached;
}
public long getTimeMillis() {
return timeMillis;
}
```
This is thread-safe without synchronization. (`testNullDateWithUpdate`then becomes meaningless, and method `isDateNull` can be removed.)
Or, since the assumption is that `getDate()` is called far less often, just
```
private volatile long timeMillis;
public void update() {
timeMillis = System.currentTimeMillis();
}
public Date getDate() {
return new Date(timeMillis);
}
public long getTimeMillis() {
return timeMillis;
}
```
Or, since you also added `getTimeMillis`, leave creating the `Date` to the caller and don't provide `getDate` at all.
At that point, all we have is a thread-safe mutable `long`. But that exists already in Java: class `AtomicLong`.
So we could do in `FtpIoSession`:
```
// In constructor:
this.wrappedSession.setAttribute(ATTRIBUTE_LAST_ACCESS_TIME, new AtomicLong(System.currentTimeMillis()));
public Date getLastAccessTime() {
return new Date(((AtomicLong) getAttribute(ATTRIBUTE_LAST_ACCESS_TIME)).get());
}
public void updateLastAccessTime() {
((AtomicLong) getAttribute(ATTRIBUTE_LAST_ACCESS_TIME)).set(System.currentTimeMillis());
}
```
and we wouldn't need the `LazyDateHolder` class at all, and it would be fully thread-safe. This would be much simpler, and probably good enough even though `getLastAccessTime()` always creates a new `Date`object. After all the assumption is that this is called infrequently.
##########
core/src/test/java/org/apache/ftpserver/util/LazyDateHolderTest.java:
##########
@@ -0,0 +1,130 @@
+package org.apache.ftpserver.util;
+
+import junit.framework.TestCase;
+
+import java.util.Date;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.atomic.AtomicReference;
+
+/**
+ * Unit tests for {@link LazyDateHolder}.
+ */
+public class LazyDateHolderTest extends TestCase {
+
+ public void testInitialTimeFromConstructor() {
+ long now = System.currentTimeMillis();
+ LazyDateHolder holder = new LazyDateHolder(now);
+ assertEquals("Initial timeMillis should match provided value", now, holder.getTimeMillis());
+ assertEquals("Initial Date should match provided value", now, holder.getDate().getTime());
+ }
+
+ public void testInitialTimeFromNoArgConstructor() {
+ LazyDateHolder holder = new LazyDateHolder();
+ long now = System.currentTimeMillis();
+ long diff = Math.abs(holder.getTimeMillis() - now);
+ assertTrue("Initial timeMillis should be close to current time", diff < 1000);
+ }
+
+ public void testUpdateWithSameTimeDoesNotChangeDate() {
+ long now = System.currentTimeMillis();
+ LazyDateHolder holder = new LazyDateHolder(now);
+ Date firstDate = holder.getDate();
+
+ holder.update(now); // update with same millis
+ Date secondDate = holder.getDate();
+
+ assertSame("Date instance should not change if timeMillis is the same", firstDate, secondDate);
+ }
+
+ public void testUpdateWithDifferentTimeChangesDate() {
+ long now = System.currentTimeMillis();
+ LazyDateHolder holder = new LazyDateHolder(now);
+ Date firstDate = holder.getDate();
+
+ holder.update(now + 1000); // advance by 1 second
+ Date secondDate = holder.getDate();
+
+ assertNotSame("Date instance should change when timeMillis changes", firstDate, secondDate);
+ assertEquals("Date should reflect updated timeMillis", now + 1000, secondDate.getTime());
+ }
+
+ public void testUpdateWithoutArgumentUsesCurrentTime() throws InterruptedException {
+ LazyDateHolder holder = new LazyDateHolder();
+ Date firstDate = holder.getDate();
+ Thread.sleep(5); // ensure time passes
+ holder.update(); // auto update
+ Date secondDate = holder.getDate();
+
+ assertTrue("Date after auto update should be newer", secondDate.getTime() > firstDate.getTime());
+ }
+
+ public void testNullDateWithUpdate() throws InterruptedException {
+ LazyDateHolder holder = new LazyDateHolder();
+ assertTrue("Initially should be null",holder.isDateNull());
+
+ Thread.sleep(5); // ensure time passes
+ holder.update(); // auto update
+ Date secondDate = holder.getDate();
+ assertFalse("Should not be null as date retrieved",holder.isDateNull());
Review Comment:
Use `assertNotNull`.
##########
core/src/main/java/org/apache/ftpserver/impl/FtpIoSession.java:
##########
@@ -124,15 +124,19 @@ public class FtpIoSession implements IoSession {
/**
* Public constructor
*
- * @param wrappedSession The wrapped IoSession
- * @param context The server cobtext
+ * @param wrappedSession
+ * The wrapped IoSession
+ * @param context
+ * The server cobtext
Review Comment:
Please adjust the formatting settings of your IDE to conform to this project's usage.
The PR should not have unrelated formatting changes.
##########
core/src/test/java/org/apache/ftpserver/util/LazyDateHolderTest.java:
##########
@@ -0,0 +1,130 @@
+package org.apache.ftpserver.util;
+
+import junit.framework.TestCase;
+
+import java.util.Date;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.atomic.AtomicReference;
+
+/**
+ * Unit tests for {@link LazyDateHolder}.
+ */
+public class LazyDateHolderTest extends TestCase {
+
+ public void testInitialTimeFromConstructor() {
+ long now = System.currentTimeMillis();
+ LazyDateHolder holder = new LazyDateHolder(now);
+ assertEquals("Initial timeMillis should match provided value", now, holder.getTimeMillis());
+ assertEquals("Initial Date should match provided value", now, holder.getDate().getTime());
+ }
+
+ public void testInitialTimeFromNoArgConstructor() {
+ LazyDateHolder holder = new LazyDateHolder();
+ long now = System.currentTimeMillis();
+ long diff = Math.abs(holder.getTimeMillis() - now);
+ assertTrue("Initial timeMillis should be close to current time", diff < 1000);
+ }
+
+ public void testUpdateWithSameTimeDoesNotChangeDate() {
+ long now = System.currentTimeMillis();
+ LazyDateHolder holder = new LazyDateHolder(now);
+ Date firstDate = holder.getDate();
+
+ holder.update(now); // update with same millis
+ Date secondDate = holder.getDate();
+
+ assertSame("Date instance should not change if timeMillis is the same", firstDate, secondDate);
+ }
+
+ public void testUpdateWithDifferentTimeChangesDate() {
+ long now = System.currentTimeMillis();
+ LazyDateHolder holder = new LazyDateHolder(now);
+ Date firstDate = holder.getDate();
+
+ holder.update(now + 1000); // advance by 1 second
+ Date secondDate = holder.getDate();
+
+ assertNotSame("Date instance should change when timeMillis changes", firstDate, secondDate);
+ assertEquals("Date should reflect updated timeMillis", now + 1000, secondDate.getTime());
+ }
+
+ public void testUpdateWithoutArgumentUsesCurrentTime() throws InterruptedException {
+ LazyDateHolder holder = new LazyDateHolder();
+ Date firstDate = holder.getDate();
+ Thread.sleep(5); // ensure time passes
+ holder.update(); // auto update
+ Date secondDate = holder.getDate();
+
+ assertTrue("Date after auto update should be newer", secondDate.getTime() > firstDate.getTime());
+ }
+
+ public void testNullDateWithUpdate() throws InterruptedException {
+ LazyDateHolder holder = new LazyDateHolder();
+ assertTrue("Initially should be null",holder.isDateNull());
+
+ Thread.sleep(5); // ensure time passes
+ holder.update(); // auto update
+ Date secondDate = holder.getDate();
+ assertFalse("Should not be null as date retrieved",holder.isDateNull());
+ Thread.sleep(5); // ensure time passes
+ holder.update(); // auto update
+ assertTrue("Should be not null due to update",holder.isDateNull());
+
+ long now = System.currentTimeMillis();
+ holder.update(now);
+ assertTrue("Should be not null due to update",holder.isDateNull());
Review Comment:
As above.
##########
core/src/test/java/org/apache/ftpserver/util/LazyDateHolderTest.java:
##########
@@ -0,0 +1,130 @@
+package org.apache.ftpserver.util;
Review Comment:
Missing license header.
##########
core/src/test/java/org/apache/ftpserver/util/LazyDateHolderTest.java:
##########
@@ -0,0 +1,130 @@
+package org.apache.ftpserver.util;
+
+import junit.framework.TestCase;
+
+import java.util.Date;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.atomic.AtomicReference;
+
+/**
+ * Unit tests for {@link LazyDateHolder}.
+ */
+public class LazyDateHolderTest extends TestCase {
+
+ public void testInitialTimeFromConstructor() {
+ long now = System.currentTimeMillis();
+ LazyDateHolder holder = new LazyDateHolder(now);
+ assertEquals("Initial timeMillis should match provided value", now, holder.getTimeMillis());
+ assertEquals("Initial Date should match provided value", now, holder.getDate().getTime());
+ }
+
+ public void testInitialTimeFromNoArgConstructor() {
+ LazyDateHolder holder = new LazyDateHolder();
+ long now = System.currentTimeMillis();
+ long diff = Math.abs(holder.getTimeMillis() - now);
+ assertTrue("Initial timeMillis should be close to current time", diff < 1000);
+ }
+
+ public void testUpdateWithSameTimeDoesNotChangeDate() {
+ long now = System.currentTimeMillis();
+ LazyDateHolder holder = new LazyDateHolder(now);
+ Date firstDate = holder.getDate();
+
+ holder.update(now); // update with same millis
+ Date secondDate = holder.getDate();
+
+ assertSame("Date instance should not change if timeMillis is the same", firstDate, secondDate);
+ }
+
+ public void testUpdateWithDifferentTimeChangesDate() {
+ long now = System.currentTimeMillis();
+ LazyDateHolder holder = new LazyDateHolder(now);
+ Date firstDate = holder.getDate();
+
+ holder.update(now + 1000); // advance by 1 second
+ Date secondDate = holder.getDate();
+
+ assertNotSame("Date instance should change when timeMillis changes", firstDate, secondDate);
+ assertEquals("Date should reflect updated timeMillis", now + 1000, secondDate.getTime());
+ }
+
+ public void testUpdateWithoutArgumentUsesCurrentTime() throws InterruptedException {
+ LazyDateHolder holder = new LazyDateHolder();
+ Date firstDate = holder.getDate();
+ Thread.sleep(5); // ensure time passes
+ holder.update(); // auto update
+ Date secondDate = holder.getDate();
+
+ assertTrue("Date after auto update should be newer", secondDate.getTime() > firstDate.getTime());
+ }
+
+ public void testNullDateWithUpdate() throws InterruptedException {
+ LazyDateHolder holder = new LazyDateHolder();
+ assertTrue("Initially should be null",holder.isDateNull());
+
+ Thread.sleep(5); // ensure time passes
+ holder.update(); // auto update
+ Date secondDate = holder.getDate();
+ assertFalse("Should not be null as date retrieved",holder.isDateNull());
+ Thread.sleep(5); // ensure time passes
+ holder.update(); // auto update
+ assertTrue("Should be not null due to update",holder.isDateNull());
+
+ long now = System.currentTimeMillis();
+ holder.update(now);
+ assertTrue("Should be not null due to update",holder.isDateNull());
+
+ Date customDate = holder.getDate();
+ holder.update(now); // same value
+ assertFalse("Should be not null as the update was the same",holder.isDateNull());
Review Comment:
Use `assertNotNull`.
##########
core/src/test/java/org/apache/ftpserver/util/LazyDateHolderTest.java:
##########
@@ -0,0 +1,130 @@
+package org.apache.ftpserver.util;
+
+import junit.framework.TestCase;
+
+import java.util.Date;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.atomic.AtomicReference;
+
+/**
+ * Unit tests for {@link LazyDateHolder}.
+ */
+public class LazyDateHolderTest extends TestCase {
+
+ public void testInitialTimeFromConstructor() {
+ long now = System.currentTimeMillis();
+ LazyDateHolder holder = new LazyDateHolder(now);
+ assertEquals("Initial timeMillis should match provided value", now, holder.getTimeMillis());
+ assertEquals("Initial Date should match provided value", now, holder.getDate().getTime());
+ }
+
+ public void testInitialTimeFromNoArgConstructor() {
+ LazyDateHolder holder = new LazyDateHolder();
+ long now = System.currentTimeMillis();
+ long diff = Math.abs(holder.getTimeMillis() - now);
+ assertTrue("Initial timeMillis should be close to current time", diff < 1000);
+ }
+
+ public void testUpdateWithSameTimeDoesNotChangeDate() {
+ long now = System.currentTimeMillis();
+ LazyDateHolder holder = new LazyDateHolder(now);
+ Date firstDate = holder.getDate();
+
+ holder.update(now); // update with same millis
+ Date secondDate = holder.getDate();
+
+ assertSame("Date instance should not change if timeMillis is the same", firstDate, secondDate);
+ }
+
+ public void testUpdateWithDifferentTimeChangesDate() {
+ long now = System.currentTimeMillis();
+ LazyDateHolder holder = new LazyDateHolder(now);
+ Date firstDate = holder.getDate();
+
+ holder.update(now + 1000); // advance by 1 second
+ Date secondDate = holder.getDate();
+
+ assertNotSame("Date instance should change when timeMillis changes", firstDate, secondDate);
+ assertEquals("Date should reflect updated timeMillis", now + 1000, secondDate.getTime());
+ }
+
+ public void testUpdateWithoutArgumentUsesCurrentTime() throws InterruptedException {
+ LazyDateHolder holder = new LazyDateHolder();
+ Date firstDate = holder.getDate();
+ Thread.sleep(5); // ensure time passes
+ holder.update(); // auto update
+ Date secondDate = holder.getDate();
+
+ assertTrue("Date after auto update should be newer", secondDate.getTime() > firstDate.getTime());
+ }
+
+ public void testNullDateWithUpdate() throws InterruptedException {
+ LazyDateHolder holder = new LazyDateHolder();
+ assertTrue("Initially should be null",holder.isDateNull());
Review Comment:
Use `assertNull`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
Re: [PR] Reduce GC Pressure by Storing Last Access Time as Primitive Long [mina-ftpserver]
Posted by "chuckbinder (via GitHub)" <gi...@apache.org>.
chuckbinder commented on PR #76:
URL: https://github.com/apache/mina-ftpserver/pull/76#issuecomment-2832808219
I apologize, my last commit failed. I have adjusted it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
Re: [PR] Reduce GC Pressure by Storing Last Access Time as Primitive Long [mina-ftpserver]
Posted by "chuckbinder (via GitHub)" <gi...@apache.org>.
chuckbinder commented on PR #76:
URL: https://github.com/apache/mina-ftpserver/pull/76#issuecomment-2833478612
I appreciate the feedback and your right just keep it simple. I apologize for the errors as this was my first pull request. I thought I had the projects formatter imported and used so I will reverify. I will close this pull request and create a new one with timings and everything proper.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org