Skip to content

Improve copy with per-resource locking #36

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

Merged
merged 5 commits into from
Mar 26, 2025

Conversation

jdisho
Copy link
Member

@jdisho jdisho commented Mar 18, 2025

Improve copy with per-resource locking

♻️ Current situation & Problem

LLMonFHIR - #79

⚙️ Release Notes

  • Implemented thread-safe deep copying for FHIR resources using per-resource locking.
  • Using PropertyListEncoder/Decoder instead of JSONEncoder/Decoder. PropertyListEncoder/Decoder should be faster than JSONEncoder/Decoder as they use binary data instead of text, skipping the need for text formatting and character escaping.

📚 Documentation

Added in code.

✅ Testing

Should:

  • Measure how much overhead our thread safety changes add to copy operations compared to the original implementation.
  • Measure if PropertyList encoding/decoding is actually faster than the current JSON approach for our FHIR resources. (Using JSONEncoder/Decoder is a better choice. Check the comments)
  • Run many copy operations simultaneously from different threads to check the thread safety

📝 Code of Conduct & Contributing Guidelines

By creating and submitting this pull request, you agree to follow our Code of Conduct and Contributing Guidelines:

@jdisho jdisho added the enhancement New feature or request label Mar 18, 2025
@jdisho jdisho requested a review from PSchmiedmayer March 18, 2025 17:35
@jdisho jdisho self-assigned this Mar 18, 2025
@jdisho jdisho requested a review from philippzagar March 18, 2025 17:35
Copy link

codecov bot commented Mar 18, 2025

Codecov Report

Attention: Patch coverage is 98.30508% with 1 line in your changes missing coverage. Please review.

Project coverage is 54.79%. Comparing base (bb7f7b5) to head (4f8a42c).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...eziFHIR/FHIRResource/FHIRResourceLockManager.swift 96.56% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #36      +/-   ##
==========================================
+ Coverage   52.94%   54.79%   +1.86%     
==========================================
  Files          31       32       +1     
  Lines        1755     1807      +52     
==========================================
+ Hits          929      990      +61     
+ Misses        826      817       -9     
Files with missing lines Coverage Δ
...ces/SpeziFHIR/FHIRResource/FHIRResource+Copy.swift 100.00% <100.00%> (+100.00%) ⬆️
...eziFHIR/FHIRResource/FHIRResourceLockManager.swift 96.56% <96.56%> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bb7f7b5...4f8a42c. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jdisho jdisho force-pushed the thread-safe-deep-copy-of-the-fhir-resource branch from dc4ff91 to d5499c8 Compare March 18, 2025 17:41
@jdisho jdisho changed the title Improve copy() with per-resource locking Improve copy with per-resource locking Mar 18, 2025
Copy link
Member

@PSchmiedmayer PSchmiedmayer left a comment

Choose a reason for hiding this comment

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

Thank you for the work her @jdisho , happy to see this merged once we added a few tests! 🚀

@jdisho
Copy link
Member Author

jdisho commented Mar 24, 2025

Ran some tests to measure the thread safety overhead. 10k iterations for each test. Using PropertyDecoder/Encoder

----- DSTU2 Resource (DocumentReference with attachments) -----
Thread-safe time: 1.6574610471725464 seconds
Non-thread-safe time: 1.6063940525054932 seconds
Thread safety overhead: 3.18%

----- DSTU2 Resource (Condition) -----
Thread-safe time: 0.6062500476837158 seconds
Non-thread-safe time: 0.5733170509338379 seconds
Thread safety overhead: 5.74%

----- DSTU2 Resource (Medication) -----
Thread-safe time: 0.1977999210357666 seconds
Non-thread-safe time: 0.17054808139801025 seconds
Thread safety overhead: 15.98%

----- Mixed Resource Types -----
Thread-safe time: 0.6122219562530518 seconds
Non-thread-safe time: 0.5736989974975586 seconds
Thread safety overhead: 6.71%

----- Multi-Thread Same Resource (Contention) (10 concurrent threads) -----
Thread-safe time (concurrent): 0.7557909488677979 seconds
Non-thread-safe time (sequential): 0.7050811052322388 seconds
Thread safety overhead: 7.19%

----- R4 Resource (DocumentReference with attachments) -----
Thread-safe time: 1.263077974319458 seconds
Non-thread-safe time: 1.2212491035461426 seconds
Thread safety overhead: 3.43%

----- R4 Resource (MedicationRequest) -----
Thread-safe time: 0.734023928642273 seconds
Non-thread-safe time: 0.6970599889755249 seconds
Thread safety overhead: 5.30%

----- R4 Resource (Patient) -----
Thread-safe time: 0.27093303203582764 seconds
Non-thread-safe time: 0.2436460256576538 seconds

@jdisho
Copy link
Member Author

jdisho commented Mar 24, 2025

10k iterations for each test. Using JSONDecoder/Encoder

----- DSTU2 Resource (DocumentReference with attachments) -----
Thread-safe time: 1.2903610467910767 seconds
Non-thread-safe time: 1.2287269830703735 seconds
Thread safety overhead: 5.02%

----- DSTU2 Resource (Condition) -----
Thread-safe time: 0.45733094215393066 seconds
Non-thread-safe time: 0.4253389835357666 seconds
Thread safety overhead: 7.52%

----- DSTU2 Resource (Medication) -----
Thread-safe time: 0.1462399959564209 seconds
Non-thread-safe time: 0.11967003345489502 seconds
Thread safety overhead: 22.20%

----- Mixed Resource Types -----
Thread-safe time: 0.47743308544158936 seconds
Non-thread-safe time: 0.4483609199523926 seconds
Thread safety overhead: 6.48%

----- Multi-Thread Same Resource (Contention) -----
Thread-safe time (concurrent): 0.5626839399337769 seconds
Non-thread-safe time (sequential): 0.5115839242935181 seconds
Thread safety overhead: 9.99%

----- R4 Resource (DocumentReference with attachments) -----
Thread-safe time: 0.9039510488510132 seconds
Non-thread-safe time: 0.8656200170516968 seconds
Thread safety overhead: 4.43%

----- R4 Resource (MedicationRequest) -----
Thread-safe time: 0.534337043762207 seconds
Non-thread-safe time: 0.5068429708480835 seconds
Thread safety overhead: 5.42%

----- R4 Resource (Patient) -----
Thread-safe time: 0.2117760181427002 seconds
Non-thread-safe time: 0.1854860782623291 seconds
Thread safety overhead: 14.17%

@jdisho
Copy link
Member Author

jdisho commented Mar 24, 2025

These measurements are not very consistent but at least they indicate that PropertyEncoder/Decoder will (moderately) improve performance, while the thread safety lock increases overhead. Overall, I think this is a reasonable compromise.

@PSchmiedmayer
Copy link
Member

PSchmiedmayer commented Mar 24, 2025

@jdisho Thank you for benchmarking the encoders!

I just looked at the numbers, let me know if I missed something but the JSONDecoder/Encoder ones all seemed smaller and I would assume smaller means faster and therefore a better performance compared to the PropertyDecoder/Encoder? So using JSON would be a better choice?

@jdisho
Copy link
Member Author

jdisho commented Mar 25, 2025

I completely overlooked the execution time, as I was focused on the overhead percentage. You are right, great catch. I ran these measurements several times, and in all of them, the execution time is smaller when using JSONEncoder/Decoder.

@PSchmiedmayer
Copy link
Member

PSchmiedmayer commented Mar 25, 2025

Nice, so using JSON Encoder/Decoder is the way to go, and the locking overhead is not huge so good signs that this combination is the way to go 👍
Thank you for looking into all of this!

@jdisho jdisho force-pushed the thread-safe-deep-copy-of-the-fhir-resource branch from 74d4d74 to ff25d28 Compare March 25, 2025 14:48
@jdisho
Copy link
Member Author

jdisho commented Mar 25, 2025

I added couple of tests, let me know what you think 🚀

Copy link
Member

@PSchmiedmayer PSchmiedmayer left a comment

Choose a reason for hiding this comment

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

Thank you for all the tests here, looks great!

@jdisho jdisho merged commit d5622db into main Mar 26, 2025
11 of 12 checks passed
@jdisho jdisho deleted the thread-safe-deep-copy-of-the-fhir-resource branch March 26, 2025 06:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants