RUM-6171 use FileObserver to limit calls to filesystem #2718
Conversation
| } | ||
|
|
||
| init { | ||
| fileObserver.startWatching() |
There was a problem hiding this comment.
should we stop watching at some point ? not sure when though or if needed
There was a problem hiding this comment.
In theory yes but not sure when either
There was a problem hiding this comment.
We could use Object.finalize, but here it is Kotlin Any, so no counterpart method.
We probably need to still find a way to stop watching, because if Datadog.stop is called, we will leak this subscription.
There was a problem hiding this comment.
Apparently we should be good, because there is stopWatching call in the FileObserver.finalize (link).
...ain/kotlin/com/datadog/android/core/internal/persistence/file/batch/BatchFileOrchestrator.kt
Outdated
Show resolved
Hide resolved
xgouchet
force-pushed
the
xgouchet/RUM-6171/FileObserver
branch
from
5c82f6f to
a467af2
Compare
0xnm
left a comment
0xnm
left a comment
There was a problem hiding this comment.
Nice change! It will reduce CPU utilization for the case when we have many files on the disk. I added a few comments.
| rootDir.listFilesSafe(internalLogger)?.toMutableSet() ?: mutableSetOf() | ||
|
|
||
| @Suppress("DEPRECATION") // Recommended constructor only available in API 29 Q | ||
| internal val fileObserver = object : FileObserver(rootDir.path, CREATE or DELETE) { |
There was a problem hiding this comment.
Mask is saying that only CREATE or DELETE events will be watched, but MOVED_TO, MOVED_FROM are mentioned in the code as well.
| MOVED_FROM, DELETE -> { | ||
| if (!name.isNullOrEmpty() && name.isBatchFileName) { | ||
| synchronized(knownFiles) { | ||
| knownFiles.removeAll { it.name == name } |
There was a problem hiding this comment.
given that knownFiles is a Set<File> and File.equals relies on the path comparison anyway (link), won't it be simpler to use .remove call instead of iterating?
| } | ||
|
|
||
| init { | ||
| fileObserver.startWatching() |
There was a problem hiding this comment.
We could use Object.finalize, but here it is Kotlin Any, so no counterpart method.
We probably need to still find a way to stop watching, because if Datadog.stop is called, we will leak this subscription.
xgouchet
force-pushed
the
xgouchet/RUM-6171/FileObserver
branch
from
a467af2 to
6d02b12
Compare
| private val knownFiles: MutableSet<File> = | ||
| rootDir.listFilesSafe(internalLogger)?.toMutableSet() ?: mutableSetOf() |
There was a problem hiding this comment.
I realized the it may bring the following concern: this will be called on the user thread during SDK initialization and since it is I/O call, it may slow down SDK start (especially if there is a lot of files on the disk).
Instead, we maybe can to initial read in the getWritableFile / getReadableFile methods (well, on any initial access of this property), this will shift it to the worker thread improving SDK initialization.
Same may apply to the root folder creation.
|
We have similar PR: #2720 |
5 participants
What does this PR do?
Use the file observer to keep an up to date list of batch files instead of calling
listFileseach time.Motivation
Performance improvement