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

Remove @Synchronized annotation from Rfc3339DateJsonAdapter. #1842

Conversation

sangho-block
Copy link
Contributor

@sangho-block sangho-block commented May 10, 2024

Motivation

Rfc3339DateJsonAdapter can convert a nullable Date object to a JSON string and vice versa.

If a system attempts to concurrently serialize or deserialize large objects containing many Date objects or collections of those large objects, there could be contention across threads and delays due to synchronization.

Screenshot 2024-05-10 at 6 04 23 PM

Synchronization evaluation

1. fromJson

fromJson constructs and uses a local Calendar instance and does not use any shared DateFormat, thus it should not need to be synchronized.

2. toJson

toJson calls Date.formatIsoDate extension method, which provides a (mutable) Date object to local Calendar instance.

internal fun Date.formatIsoDate(): String {
  val calendar: Calendar = GregorianCalendar(TIMEZONE_Z, Locale.US)
  calendar.time = this
  // ...
}

The underlying code for calendar.time = this simply gets the Long time value from the Date object which is a thread-safe operation.

  public final void setTime(Date date) {
     Objects.requireNonNull(date, "date must not be null");
     setTimeInMillis(date.getTime());
  }

Therefore, synchronized execution shouldn't be needed for toJson.

Changes

  • Remove @Synchronized annotation from fromJson and toJson in Rfc3339DateJsonAdapter to make it usable concurrently.

Outcome

Lower contention across threads & synchronization overhead can improve efficiency and compute resource utilization in multi-threaded environments such as in backend servers.

Benchmark

Setup

  • M1 Max MBP
  • 100 fixed threads
  • 1000 serialization and deserialization tasks
    • Each object with a collection of 100 objects with date property.

Outcome

Benchmark                         Mode  Cnt   Score    Error  Units
BenchmarkDate.synchronized       thrpt    5  10.707  ± 0.052  ops/s   // before
BenchmarkDate.non-synchronized   thrpt    5  77.978  ± 5.109  ops/s   // after

@sangho-block sangho-block changed the title Rfc3339datejsonadapter/remove synchronized Make Rfc3339DateJsonAdapter thread safe & remove @Synchronized annotation. May 11, 2024
@sangho-block sangho-block changed the title Make Rfc3339DateJsonAdapter thread safe & remove @Synchronized annotation. Remove @Synchronized annotation from Rfc3339DateJsonAdapter. May 11, 2024
@sangho-block sangho-block marked this pull request as ready for review May 22, 2024 19:38
Copy link

@dreamofabear dreamofabear left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding a comment explicitly stating that these should NOT be synchronized to avoid accidental future regression.

@JakeWharton JakeWharton enabled auto-merge (squash) May 28, 2024 16:46
@JakeWharton JakeWharton merged commit 892a4d9 into square:master May 28, 2024
3 checks passed
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.

None yet

4 participants