-
Notifications
You must be signed in to change notification settings - Fork 75
RUM-6171 use FileObserver to limit calls to filesystem #2720
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
base: develop
Are you sure you want to change the base?
Conversation
6d02b12 to
ecb5cc6
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2720 +/- ##
===========================================
+ Coverage 71.28% 71.31% +0.03%
===========================================
Files 880 880
Lines 32360 32372 +12
Branches 5457 5461 +4
===========================================
+ Hits 23066 23083 +17
- Misses 7736 7745 +9
+ Partials 1558 1544 -14
🚀 New features to boost your workflow:
|
3fbb972
ecb5cc6 to
3fbb972
Compare
|
🎯 Code Coverage 🔗 Commit SHA: 441a603 | Docs | Datadog PR Page | Was this helpful? Give us feedback! |
fc7afba to
0e1acc1
Compare
afa1dd3 to
d5d41e3
Compare
b257b6c to
441a603
Compare
0xnm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! It is worth to do some benchmarks to compare the old and new ways of handling batch files in different scenarios. For example, when there is no batch files, 100 batch files, 10000 batch files.
| @Synchronized | ||
| private fun startFileObserverIfNotStarted() { | ||
| if (isFileObserverStarted.compareAndSet(false, true)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory CAS operation isFileObserverStarted.compareAndSet(false, true) already guards the logical branch from parallel execution, so @Synchronized is not needed. Do I miss something?
|
|
||
| @Suppress("LiftReturnOrAssignment", "ReturnCount") | ||
| @Suppress("ReturnCount") | ||
| private fun isRootDirValid(): Boolean { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be called from getRootDir <- ... <- SdkFeature.prepareStorage, meaning it will be called on the caller thread. In most of the cases it will be main thread during app startup, so we need to make sure that these changes don't have significant impact perf-wise at the SDK initialization.
What does this PR do?
The same as #2718, just branch moved directly to the repo.
Review checklist (to be filled by reviewers)