RUM-16056 Add @HotMethod annotation and HotMethodIllegalCall detekt rule#3558
RUM-16056 Add @HotMethod annotation and HotMethodIllegalCall detekt rule#3558satween wants to merge 1 commit into
@HotMethod annotation and HotMethodIllegalCall detekt rule#3558Conversation
Introduces a source-level @HotMethod annotation to mark frame/touch hot-path methods, a new detekt rule (HotMethodIllegalCall) that forbids heap allocations and O(N) collection operations inside them, and a dedicated detekt config (detekt_expensive_methods.yml). Annotates existing hot-path methods in RUM, Session Replay, and Compose and pre-allocates objects to suppress legitimate findings. Ref: RUM-16056
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 72ec5c04a8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| private val MESSAGE_MOTION_EVENT_WAS_NULL: () -> String = { | ||
| "RecorderWindowCallback: intercepted null motion event" | ||
| } |
There was a problem hiding this comment.
This replaces the internal MOTION_EVENT_WAS_NULL_ERROR_MESSAGE constant with a private lambda, but features/dd-sdk-android-session-replay/src/test/kotlin/com/datadog/android/sessionreplay/internal/recorder/callback/RecorderWindowCallbackTest.kt:232 still references RecorderWindowCallback.MOTION_EVENT_WAS_NULL_ERROR_MESSAGE. As-is, the session-replay test sources no longer compile; keep the constant available or update the test while still reusing a preallocated message builder.
Useful? React with 👍 / 👎.
| "invocation. Move the allocation to a field or pre-allocate outside the hot path." | ||
| ) | ||
| ) | ||
| return |
There was a problem hiding this comment.
Returning here prevents super.visitCallExpression from descending into constructor arguments, so any illegal calls nested inside an unavoidable/suppressed result allocation are silently skipped. For example, @Suppress("HotMethodIllegalCall") Foo(list.map { ... }) in a @HotMethod would suppress the constructor finding and the rule would never visit the map call or its lambda; report the constructor but still traverse children.
Useful? React with 👍 / 👎.
|
| private val insideHotMethod: Boolean | ||
| get() = functionDepthStack.lastOrNull() == true | ||
|
|
||
| override fun visitNamedFunction(function: KtNamedFunction) { |
There was a problem hiding this comment.
Question, this detekt rule checks only the hot method body but does not recursively apply the call tree, meaning that the rule can be escaped by extracting the expensive call into another function, is this expected?
@HotMethod
fun foo(){
bar()
}
fun bar(){
// expensive calls here
}
| "invocation. Move the allocation to a field or pre-allocate outside the hot path." | ||
| ) | ||
| ) | ||
| return |
| */ | ||
| @Retention(AnnotationRetention.SOURCE) | ||
| @Target(AnnotationTarget.FUNCTION) | ||
| annotation class HotMethod(val message: String) |
There was a problem hiding this comment.
is it possible to add a ignore list in the params? I can imagine in some case we need to ignore some rules, instead of Suppress the whole HotMethod rule, we can bypass only a few but apply the rest?
| listOf(InternalLogger.Target.MAINTAINER, InternalLogger.Target.TELEMETRY), | ||
| { "Received null MotionEvent" } | ||
| ERROR_LOG_TARGETS, | ||
| MESSAGE_NULL_MOTION_EVENT |
There was a problem hiding this comment.
Question, if such message requires a variable from the @HotMethod, and must be constructed such as:
internalLogger.log(
InternalLogger.Level.ERROR,
ERROR_LOG_TARGETS,
messageBuilder = { MESSAGE_TIMESTAMP.format(Locale.US, event.time) }
)
It's expected that we throw the error for this?
| override fun visitLambdaExpression(expression: KtLambdaExpression) { | ||
| super.visitLambdaExpression(expression) | ||
| if (!insideHotMethod) return | ||
| if (expression.isInlinedLambda()) return |
There was a problem hiding this comment.
This only excludes the case where the lambda is inlined, but the lambda which captures nothing can also be ignored since it will be optimized during the Kotlin compilation, and will not be allocated everytime.
| # Lambda literals passed to functions in `allowedInlineFunctions` are inlined by the compiler | ||
| # and do not allocate — they are excluded from the lambda-allocation check. | ||
| # Add any project-specific `inline` functions here. | ||
| allowedInlineFunctions: |
There was a problem hiding this comment.
Are the functions listed in allowedInlineFunctions allowed, so the linter will not complain?
Asking because IIUC for example filter is both in allowedInlineFunctions and in forbiddenCalls.
Or am I missing the point?
3 participants
What does this PR do?
Introduces a
@HotMethodsource annotation to mark frame/touch hot-path methods and aHotMethodIllegalCalldetekt rule that forbids heap allocations and O(N) collection operations inside them. Adds a dedicateddetekt_expensive_methods.ymlconfig to activate the rule. Annotates existing hot-path methods in RUM, Session Replay, and Compose; pre-allocates objects where needed to satisfy the rule.Motivation
Methods called on every frame or touch event (JankStats callbacks,
dispatchTouchEvent,onDraw) are a common source of GC pressure and UI jank. Making them machine-verifiable via a detekt rule prevents regressions from creeping back in during normal development.Additional Notes
The
@Suppress("HotMethodIllegalCall")annotations on two sites (oneSlowFrameRecordallocation and oneTargetNodeallocation) document intentional exceptions: both allocations are bounded by actual jank/tap events rather than frame rate, so suppression is intentional and annotated with an explanation comment.Review checklist (to be filled by reviewers)
Ref: RUM-16056