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.

Sign up for GitHub

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

Jump to bottom

perf: Prewarmed app start detection #2151

Merged
merged 3 commits into from
Sep 14, 2022
Merged

perf: Prewarmed app start detection #2151

merged 3 commits into from
Sep 14, 2022

Conversation

philipphofmann
Copy link
Member

@philipphofmann philipphofmann commented Sep 13, 2022 •
edited
Loading

📜 Description

Use isEqualToString instead of isEqual. This could potentially lead to wrong prewarmed app start detection.

Update: As pointed out in #2151 (comment) this change won't fix anything. Instead, it is a performance improvement.

💡 Motivation and Context

It seems like the logic is not working properly. Still saw a few pre-warmed app starts in customer data. Maybe this is the root cause.

💚 How did you test it?

Unit tests use the same string constant so it won't fail.

📝 Checklist

  • I reviewed the submitted code
  • I added tests to verify the changes
  • I updated the docs if needed
  • Review from the native team if needed
  • No breaking changes

🔮 Next steps

@philipphofmann
fix: Prewarmed app start detection
8a8114a 
Use isEqualToString instead of isEqual. This could potentially lead to wrong
prewarmed app start detection.
@github-actions
Copy link

github-actions bot commented Sep 13, 2022 •
edited
Loading

Performance metrics 🚀

Plain With Sentry Diff
Startup time (ms) 1273.80 1275.62 1.82
Size (bytes) 21157 333596 312439

armcknight
armcknight approved these changes Sep 13, 2022
View reviewed changes
Copy link
Member

@armcknight armcknight left a comment

Choose a reason for hiding this comment

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

LGTM. By any chance did you confirm that this is still in the process environment variables with iOS 16?

@philipphofmann
Copy link
Member Author

philipphofmann commented Sep 14, 2022 •
edited
Loading

LGTM. By any chance did you confirm that this is still in the process environment variables with iOS 16?

No, I didn't find anything according to that yet. Do you maybe know, @armcknight? I will also look into customer data to find out.

@philipphofmann philipphofmann merged commit eefdeed into master Sep 14, 2022
@philipphofmann philipphofmann deleted the fix/pre-warmed-app-start-detection branch September 14, 2022 06:36
@armcknight
Copy link
Member

Not sure yet, haven't tried out Xcode 14 yet. Excited to! Will look tomorrow.

kevinrenskers added a commit that referenced this pull request Sep 15, 2022
@kevinrenskers
Merge branch 'master' into fix/1072-fix-double-quote-warnings
e05b919 
* master: (73 commits)
  ref: Fix typo in measurement (#2154)
  Fix typos in changelog (#2153)
  docs: Add a note on flaky tests to Contributing (#2161)
  docs: fix PR reference in CHANGELOG (#2157)
  test: generate graphs for benchmarks (#2158)
  release: 7.25.1
  fix: Prewarmed app start detection (#2151)
  ci: temporarily disable scheduled CI runs uploading profiles/symbols (#2152)
  Run tests on iOS 16 using Xcode 14.0 (#2147)
  ref: Fix Xcode 14 compile issues (#2145)
  Provide fallbacks for assertions in production (#2141)
  Fix compile errors in Xcode 14 (#2146)
  release: 7.25.0
  fix: setting SDK name through options[sdk][name] shouldn't clear version (#2139)
  fix: SentryTracer instances should be called "tracer", not "transaction" (#2137)
  ref: functions to convert to/from enum cases (#2108)
  fix: Crash with screenshot is reported twice (#2134)
  meta: Clarify PR rules in Contributing (#2133)
  meta: Fix Changelog (#2136)
  feat: Add support for dynamic library (#1726)
  ...
kevinrenskers added a commit that referenced this pull request Sep 15, 2022
@kevinrenskers
Merge branch 'master' into fix/fix-flaky-tests
8c4ee78 
* master:
  ref: Fix typo in measurement (#2154)
  Fix typos in changelog (#2153)
  docs: Add a note on flaky tests to Contributing (#2161)
  docs: fix PR reference in CHANGELOG (#2157)
  test: generate graphs for benchmarks (#2158)
  release: 7.25.1
  fix: Prewarmed app start detection (#2151)
  ci: temporarily disable scheduled CI runs uploading profiles/symbols (#2152)
@triplef
Copy link
Contributor

triplef commented Sep 15, 2022

I don’t see how this would fix anything, as isEqualToString: is documented to be just a faster way to check equality if one knows that both objects are strings:

When you know both objects are strings, this method is a faster way to check equality than isEqual:.

The result should be identical to calling isEqual:.

@philipphofmann
Copy link
Member Author

You are right, @triplef. Thanks, for pointing out that detail. This PR should be a refactoring or a performance improvement. I'm going to update the changelog.

@philipphofmann philipphofmann changed the title fix: Prewarmed app start detection perf: Prewarmed app start detection Sep 16, 2022
@github-actions
Copy link

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 under the following heading:

To the changelog entry, please add a link to this PR (consider a more descriptive message):`

- Prewarmed app start detection(#2151)

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

Generated by 🚫 dangerJS against 0ef6b3d

philipphofmann added a commit that referenced this pull request Sep 16, 2022
@philipphofmann
meta: Update Changelog
d811b84 
The change in #2151 doesn't fix anything, 
see #2151 (comment). 
Instead, the PR is only a performance improvement. Update the Changelog to clarify
expectation.
@philipphofmann philipphofmann mentioned this pull request Sep 16, 2022
Merged
philipphofmann added a commit that referenced this pull request Sep 16, 2022
@philipphofmann
meta: Update Changelog (#2178)
4e17cff 
The change in #2151 doesn't fix anything, 
see #2151 (comment). 
Instead, the PR is only a performance improvement. Update the Changelog to clarify
expectation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@armcknight armcknight armcknight approved these changes

@brustolin brustolin Awaiting requested review from brustolin

@indragiek indragiek Awaiting requested review from indragiek

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@philipphofmann @armcknight @triplef