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

refactor: use breadth-first search in widget-filter #2481

Merged
merged 1 commit into from
Dec 11, 2024

Conversation

vaind
Copy link
Collaborator

@vaind vaind commented Dec 11, 2024

📜 Description

Stack traces in the widget filters can get pretty deep (I've seen 800+ frames on an app that wasn't crazy complicated) so while we don't seem to be hitting any stack-depth limits, better not tempt that and let's flatten the recursion instead.

#skip-changelog

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • No new PII added or SDK only sends newly added PII if sendDefaultPii is enabled
  • I updated the docs if needed
  • All tests passing
  • No breaking changes

🔮 Next steps

Copy link
Contributor

🚨 Detected changes in high risk code 🚨

High-risk code has higher potential to break the SDK and may be hard to test. To prevent severe bugs, apply the rollout process for releasing such changes and be extra careful when changing and reviewing these files:

  • flutter/lib/src/screenshot/widget_filter.dart

Copy link

codecov bot commented Dec 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.52%. Comparing base (ab99a31) to head (a05abc6).
Report is 23 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2481      +/-   ##
==========================================
+ Coverage   86.95%   91.52%   +4.56%     
==========================================
  Files         264       87     -177     
  Lines        9385     3044    -6341     
==========================================
- Hits         8161     2786    -5375     
+ Misses       1224      258     -966     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@vaind vaind marked this pull request as ready for review December 11, 2024 12:01
Copy link
Contributor

Android Performance metrics 🚀

  Plain With Sentry Diff
Startup time 457.64 ms 493.80 ms 36.16 ms
Size 6.49 MiB 7.57 MiB 1.08 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
74e1fdd 398.61 ms 418.43 ms 19.82 ms
b49bf00 323.46 ms 375.35 ms 51.89 ms
d0312c9 354.78 ms 411.53 ms 56.75 ms
7f2b01d 304.94 ms 345.71 ms 40.78 ms
521cfbf 332.78 ms 376.04 ms 43.26 ms
039058a 448.52 ms 512.55 ms 64.03 ms
24b6e60 440.64 ms 557.96 ms 117.32 ms
33d0587 308.79 ms 370.86 ms 62.07 ms
2b9937e 439.68 ms 478.38 ms 38.70 ms
f056db1 426.04 ms 475.76 ms 49.72 ms

App size

Revision Plain With Sentry Diff
74e1fdd 6.33 MiB 7.27 MiB 954.12 KiB
b49bf00 6.06 MiB 7.10 MiB 1.04 MiB
d0312c9 6.33 MiB 7.26 MiB 949.76 KiB
7f2b01d 5.94 MiB 6.95 MiB 1.01 MiB
521cfbf 5.94 MiB 6.97 MiB 1.03 MiB
039058a 6.52 MiB 7.59 MiB 1.06 MiB
24b6e60 6.33 MiB 7.26 MiB 950.14 KiB
33d0587 6.16 MiB 7.14 MiB 1007.47 KiB
2b9937e 6.49 MiB 7.56 MiB 1.07 MiB
f056db1 6.52 MiB 7.61 MiB 1.09 MiB

Copy link
Contributor

iOS Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1244.02 ms 1267.06 ms 23.04 ms
Size 8.38 MiB 9.78 MiB 1.41 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
7f14ddd 1247.36 ms 1269.89 ms 22.53 ms
d5f600b 1220.79 ms 1232.93 ms 12.14 ms
9f9dd52 1364.63 ms 1394.89 ms 30.25 ms
873fb42 1261.00 ms 1285.92 ms 24.92 ms
8e133ad 1268.19 ms 1277.37 ms 9.18 ms
752e1cb 1250.00 ms 1267.84 ms 17.84 ms
202b83f 1206.16 ms 1225.33 ms 19.16 ms
4477d2e 1203.00 ms 1225.90 ms 22.90 ms
7c7c64f 1243.85 ms 1272.30 ms 28.45 ms
464b4d0 1249.02 ms 1261.22 ms 12.20 ms

App size

Revision Plain With Sentry Diff
7f14ddd 8.33 MiB 9.64 MiB 1.31 MiB
d5f600b 8.32 MiB 9.38 MiB 1.05 MiB
9f9dd52 8.33 MiB 9.63 MiB 1.29 MiB
873fb42 8.16 MiB 9.17 MiB 1.01 MiB
8e133ad 8.10 MiB 9.16 MiB 1.07 MiB
752e1cb 8.38 MiB 9.75 MiB 1.37 MiB
202b83f 8.33 MiB 9.40 MiB 1.07 MiB
4477d2e 8.32 MiB 9.38 MiB 1.06 MiB
7c7c64f 8.38 MiB 9.75 MiB 1.37 MiB
464b4d0 8.10 MiB 9.16 MiB 1.07 MiB

@buenaflor
Copy link
Contributor

using a dequeue I guess it boils down to whether this makes a difference right?

final currentList = _visitList;
_visitList = [];

// vs 

final element = queue.removeFirst;

@vaind
Copy link
Collaborator Author

vaind commented Dec 11, 2024

using a dequeue I guess it boils down to whether this makes a difference right?

final currentList = _visitList;
_visitList = [];

// vs 

final element = queue.removeFirst;

Yep. This function is a hotspot so it would make sense to measure. We don't currently have any (micro)benchmarks in the repo but I was wanting to add some for a long time now

@vaind vaind merged commit 04211b9 into main Dec 11, 2024
56 of 57 checks passed
@vaind vaind deleted the refactor/bfs-widget-filter branch December 11, 2024 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants