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

[tests] Add a performance test using BenchmarkDotNet. #11298

Merged
merged 6 commits into from
Apr 23, 2021

Conversation

rolfbjarne
Copy link
Member

No description provided.

@rolfbjarne rolfbjarne added the not-notes-worthy Ignore for release notes label Apr 22, 2021
Copy link
Contributor

@spouliot spouliot left a comment

Choose a reason for hiding this comment

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

I ❤️ it, but it needs a README.md, minimally

  • how to run the tests
  • how to add new tests

@vs-mobiletools-engineering-service2
Copy link
Collaborator

❌ Tests failed on Build ❌

Tests failed on Build.

API diff

✅ API Diff from stable

View API diff

Test results

3 tests failed, 101 tests passed.

Failed tests

  • framework-test/Mac Catalyst/Debug: TimedOut (Execution timed out after 1200 seconds.
    No test log file was produced)
  • monotouch-test/tvOS - simulator/Debug [dotnet]: Failed
  • link all/tvOS - simulator/Debug: Failed

Pipeline on Agent XAMBOT-1104.BigSur'
Merge 5448fbe into 94caf96

@vs-mobiletools-engineering-service2
Copy link
Collaborator

✅ Tests passed on Build. ✅

Tests passed on Build.

API diff

✅ API Diff from stable

View API diff

🎉 All 104 tests passed 🎉

Pipeline on Agent XAMBOT-1104.BigSur
Merge b437ee2 into b1080ee

@rolfbjarne rolfbjarne merged commit d15f683 into xamarin:main Apr 23, 2021
@rolfbjarne rolfbjarne deleted the perftest branch April 23, 2021 11:30
rolfbjarne added a commit that referenced this pull request Apr 23, 2021
…de. (#11271)

Move the xamarin_create_managed_ref internal call to managed code, to ease things
with CoreCLR.

In order to preserve performance, this wasn't a straight forward port.

* monotouch_create_managed_ref used to detect if there already was a GCHandle for
  a native object. To avoid a managed->native transition, this logic has now been
  moved into the code that sets the GCHandle (the xamarinSetGCHandle:flags: / xamarin_set_gchandle_trampoline
  code), and these methods return a value saying whether the GCHandle was set or
  not.

* xamarin_create_gchandle will check the retain count to determine whether to create
  a weak or a strong GCHandle for the managed object. In this particular case we
  should never need to create a strong GCHandle, which means that we don't need to
  check the retain count (saving a managed->native transition).

Using the new perftest (#11298), I get very similar numbers for both old code and new code: https://gist.github.com/rolfbjarne/e0fc2ae0f21da15062b4f051138679af (multiple runs). Sometimes the old code is faster, sometimes the new code is faster (although the old code tends to be the one who wins).

In any case there aren't any significant performance hits due to this change, so it should be good to go.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
not-notes-worthy Ignore for release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants