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

RUM-6263 Trigger based approach #2074

Open
wants to merge 26 commits into
base: develop
Choose a base branch
from

Conversation

maciejburda
Copy link
Member

@maciejburda maciejburda commented Oct 3, 2024

What and why?

This PR changes the default behaviour of the SR recorder from scheduler to trigger based approach. Significantly reducing the SR footprint on idle.

How?

It's achieved by introducing RecordingTrigger which is responsible for subscribing to two events: UIView.layoutSubviews and UIApplication.sendEvent using obj-c swizzling. Trigger takes a weak dependency on the recording coordinator to start/stop it, as well as invoke captureNextRecord.

Those events happen frequently (every touch interaction and each view's layout change), they are throttled for 100ms (which matches scheduler capture frequency).

Below you can find screenshots from the basic Xcode profiler showcasing CPU usage for similar scenario (go through checkout and stay idle on dashboard):

Before
Screenshot 2024-10-09 at 13 15 16

After
Screenshot 2024-10-09 at 13 09 47

Review checklist

  • Feature or bugfix MUST have appropriate tests (unit, integration)
  • Make sure each commit and the PR mention the Issue number or JIRA reference
  • Add CHANGELOG entry for user facing changes

@datadog-datadog-prod-us1
Copy link

datadog-datadog-prod-us1 bot commented Oct 4, 2024

Datadog Report

Branch report: maciey/RUM-6263-trigger-based-recording
Commit report: 8450ffd
Test service: dd-sdk-ios

❌ 1 Failed (1 Known Flaky), 192 Passed, 3147 Skipped, 17.27s Total duration (1m 47.66s time saved)

❌ Failed Tests (1)

  • testWhenEnabled_itSendsConfigurationTelemetry - SessionReplayTests - ❄️ Known flaky - Details

    Expand for error
     Assertion Failure at SessionReplayTests.swift:216: XCTUnwrap failed: expected non-nil value of type "ConfigurationTelemetry"
    

@maciejburda maciejburda marked this pull request as ready for review October 9, 2024 11:01
@maciejburda maciejburda requested review from a team as code owners October 9, 2024 11:01
Copy link
Member

@ncreated ncreated left a comment

Choose a reason for hiding this comment

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

Great optimization idea 👍 💯 . I left few feedbacks, but most importantly a change request to offload the main thread from unnecessary tasks.

@maciejburda maciejburda force-pushed the maciey/RUM-6263-trigger-based-recording branch from ce52eaa to 2c0baea Compare October 22, 2024 12:47
@maciejburda maciejburda changed the base branch from develop to performance_improvements October 22, 2024 12:47
@maciejburda
Copy link
Member Author

I have addressed all the feedback, which led to small refactor. But I think it makes more sense now.

Key change is that trigger is a dependency of coordinator and it notifies it through a delegate pattern.

On checking shouldSkipFrame outside of queue. I made some tests and it's significantly more performant when done from within the queue.
Screenshot 2024-10-23 at 13 37 00
Screenshot 2024-10-23 at 13 38 20

@maciejburda maciejburda requested review from ncreated and maxep October 23, 2024 12:44
Copy link
Member

@maxep maxep left a comment

Choose a reason for hiding this comment

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

Looks good 👍 I have left some suggestions and a change-request regard thread safety

@maciejburda
Copy link
Member Author

maciejburda commented Oct 23, 2024

Addressed all the requested changes, and did some extra tests (internal).

SR Enabled:
Screenshot 2024-10-23 at 15 39 04
SR Disabled:
Screenshot 2024-10-23 at 15 40 40

@maciejburda maciejburda requested a review from maxep October 23, 2024 14:46
@maciejburda maciejburda changed the base branch from performance_improvements to develop October 28, 2024 12:20
@maciejburda maciejburda force-pushed the maciey/RUM-6263-trigger-based-recording branch 2 times, most recently from 2ada16b to dd947ad Compare October 28, 2024 14:19
maxep
maxep previously approved these changes Oct 29, 2024
Copy link
Member

@maxep maxep left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for the changes 🙏

Copy link
Member

@mariedm mariedm left a comment

Choose a reason for hiding this comment

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

Awesome work! And this CPU improvement is very welcome! 👏

General question: Since we're fully transitioning to swizzling for SR recording, should we consider updating the documentation to add warnings for customers? E.g., potential conflicts with other SDKs or their own code that might also be using swizzling.

@maciejburda maciejburda force-pushed the maciey/RUM-6263-trigger-based-recording branch 2 times, most recently from 198422b to ec17e2f Compare October 30, 2024 12:42
maxep
maxep previously approved these changes Oct 31, 2024
@maxep maxep force-pushed the maciey/RUM-6263-trigger-based-recording branch from 9019d8d to 3d6060e Compare October 31, 2024 16:02
maxep
maxep previously approved these changes Nov 4, 2024
@maxep maxep force-pushed the maciey/RUM-6263-trigger-based-recording branch from 3d6060e to 3bbdf54 Compare November 4, 2024 13:15
maxep
maxep previously approved these changes Nov 4, 2024
@maciejburda maciejburda force-pushed the maciey/RUM-6263-trigger-based-recording branch from 9b3a246 to 95157bc Compare November 13, 2024 13:37
@maciejburda
Copy link
Member Author

We decided to slightly revamp the approach. Closing this one for the time being.

@maciejburda maciejburda reopened this Nov 19, 2024
@maciejburda maciejburda marked this pull request as draft November 19, 2024 10:42
@maciejburda maciejburda self-assigned this Nov 22, 2024
@maciejburda maciejburda marked this pull request as ready for review November 29, 2024 13:39
@maciejburda maciejburda requested a review from mariedm November 29, 2024 13:40
@maciejburda maciejburda marked this pull request as draft November 29, 2024 13:41
@maciejburda
Copy link
Member Author

Just noticed I need to add tests for the new swizzlers. On it. Will reopen when done.

@maciejburda maciejburda marked this pull request as ready for review December 11, 2024 14:05
Copy link
Member

@maxep maxep left a comment

Choose a reason for hiding this comment

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

It looks good, I like the split between RecordingCoordinator and RecordingTrigger 👍

What's less clear to me is the various method that we swizzle, it seems empirical and I'm not sure why we need so many. For example, CALayer.draw seems sufficient to catch all rendering, then why do we need CALayer.setNeedsDisplay and UIView.layoutSubviews? In this context, UIViewController.viewDidAppear seems redundant.

Also, I think we should let the previous swizzled implementation execute and then notify to coordinator. To not delay the CoreGraphics rendering and record the new layout.

I also left some minor question/nit but these 2 points are the most important.

@maxep maxep self-requested a review December 13, 2024 10:55
Copy link
Member

@maxep maxep left a comment

Choose a reason for hiding this comment

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

Lets address the comments above.

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.

4 participants