Skip to content

Conversation

@lcian
Copy link
Member

@lcian lcian commented Mar 10, 2025

📜 Description

CopyOnWriteArrayList allows us to access its elements without copying and without the possibility to cause a ConcurrentModificationException.
This PR modifies its usages so that we avoid copying in order to get better performance memory-wise.

We also make sure that we are using it correctly to avoid other issues (using the iterator to avoid a possible NPE that could happen with index-based accesses, and other cases where we do things that are not thread safe).

💡 Motivation and Context

Closes #2815

💚 How did you test it?

Tested as documented in #2815 (comment)

📝 Checklist

  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • I updated the wizard if needed.
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

@lcian lcian marked this pull request as draft March 10, 2025 16:36
@github-actions
Copy link
Contributor

github-actions bot commented Mar 10, 2025

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 406.61 ms 429.55 ms 22.94 ms
Size 1.58 MiB 2.21 MiB 642.36 KiB

Previous results on branch: lcian/fix/cowal-iterator

Startup times

Revision Plain With Sentry Diff
9b49778 394.02 ms 455.04 ms 61.02 ms
805cbe5 411.64 ms 526.35 ms 114.71 ms
ac1dee2 391.74 ms 412.60 ms 20.86 ms
beceaa2 410.46 ms 451.02 ms 40.56 ms
ed84ad9 386.57 ms 392.58 ms 6.02 ms

App size

Revision Plain With Sentry Diff
9b49778 1.58 MiB 2.21 MiB 642.11 KiB
805cbe5 1.58 MiB 2.21 MiB 642.07 KiB
ac1dee2 1.58 MiB 2.21 MiB 642.34 KiB
beceaa2 1.58 MiB 2.21 MiB 642.11 KiB
ed84ad9 1.58 MiB 2.21 MiB 642.35 KiB

@lcian lcian changed the title Avoid copying and iterate correctly on SentryTracer.children Fix misuses of CopyOnWriteArrayList Mar 10, 2025
@lcian
Copy link
Member Author

lcian commented Mar 10, 2025

Only found another occurrence where we are misusing CopyOnWriteArrayList.

@lcian lcian marked this pull request as ready for review March 10, 2025 17:10
@lcian lcian requested a review from adinauer March 11, 2025 08:10
@lcian lcian marked this pull request as draft March 11, 2025 10:44
@lcian lcian force-pushed the lcian/fix/cowal-iterator branch from acbfe6f to 04c2e16 Compare March 13, 2025 12:46
@lcian lcian force-pushed the lcian/fix/cowal-iterator branch from a803296 to 71e9529 Compare March 13, 2025 12:59
@lcian lcian marked this pull request as ready for review March 13, 2025 13:02
Copy link
Member

@adinauer adinauer left a comment

Choose a reason for hiding this comment

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

LGTM!

Co-authored-by: Alexander Dinauer <adinauer@users.noreply.github.com>
@lcian lcian enabled auto-merge (squash) March 14, 2025 07:48
@lcian lcian disabled auto-merge March 14, 2025 07:50
@lcian lcian merged commit 9fba6e3 into main Mar 14, 2025
35 checks passed
@lcian lcian deleted the lcian/fix/cowal-iterator branch March 14, 2025 09:18
@github-actions
Copy link
Contributor

Fails
🚫 Please consider adding a changelog entry for the next release.

Instructions and example for changelog

Please add an entry to CHANGELOG.md to the "Unreleased" section. Make sure the entry includes this PR's number.

Example:

## Unreleased

- Fix misuses of `CopyOnWriteArrayList` ([#4247](https://github.com/getsentry/sentry-java/pull/4247))

If none of the above apply, you can opt out of this check by adding #skip-changelog to the PR description.

Generated by 🚫 dangerJS against f56abbe

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.

Prevent SentryTracer.getLatestActiveSpan from copying all child spans to avoid OOMs

4 participants