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

Update caches systems references to be actual caches and not an additional 0(n) lookup #10606

Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
95 changes: 83 additions & 12 deletions Assets/MRTK/Core/Utilities/CoreServices.cs
Original file line number Diff line number Diff line change
Expand Up @@ -20,64 +20,94 @@ namespace Microsoft.MixedReality.Toolkit
/// </summary>
public static class CoreServices
{
private static IMixedRealityBoundarySystem boundarySystem;
Copy link
Contributor

Choose a reason for hiding this comment

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

We investigated this approach a while back (CC @MaxWang-MS) and decided against it, but I don't remember the exact reason now...

If we go this approach now, we'll probably want to clear these new caches in ResetCacheReference and ResetCacheReferences

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll look to update it and clear the caches from that call. Although I am concerned why you would even need to reset the caches as all the systems listed in this function are transient, meaning they should never be cleared for the lifetime of the runtime.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated as per your request @keveleigh

Copy link
Contributor

Choose a reason for hiding this comment

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

all the systems listed in this function are transient, meaning they should never be cleared for the lifetime of the runtime.

Just to make sure we're talking about the same thing, my understanding of "transient" and "should never be cleared for the lifetime of the runtime" are two different things: the first is short-lived, and the other sounds more long-lived.

Either way though, we don't assert that MRTK services are specifically one or the other! Devs may swap profiles at runtime, leading to these cached references potentially going stale. The cache clearing logic all happens in MixedRealityToolkit.cs when the profile is changed and the services are torn down and spun back up.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm also going to investigate cherry-picking this to 2.8.1. I think this will help with a selection of perf issues I've been strategically targeting. Thanks!


/// <summary>
/// Cached reference to the active instance of the boundary system.
/// If system is destroyed, reference will be invalid. Please use ResetCacheReferences()
/// </summary>
public static IMixedRealityBoundarySystem BoundarySystem => GetService<IMixedRealityBoundarySystem>();
public static IMixedRealityBoundarySystem BoundarySystem => boundarySystem ?? (boundarySystem = GetService<IMixedRealityBoundarySystem>());

private static IMixedRealityCameraSystem cameraSystem;

/// <summary>
/// Cached reference to the active instance of the camera system.
/// If system is destroyed, reference will be invalid. Please use ResetCacheReferences()
/// </summary>
public static IMixedRealityCameraSystem CameraSystem => GetService<IMixedRealityCameraSystem>();
public static IMixedRealityCameraSystem CameraSystem => cameraSystem ?? (cameraSystem = GetService<IMixedRealityCameraSystem>());

private static IMixedRealityDiagnosticsSystem diagnosticsSystem;

/// <summary>
/// Cached reference to the active instance of the diagnostics system.
/// If system is destroyed, reference will be invalid. Please use ResetCacheReferences()
/// </summary>
public static IMixedRealityDiagnosticsSystem DiagnosticsSystem => GetService<IMixedRealityDiagnosticsSystem>();
public static IMixedRealityDiagnosticsSystem DiagnosticsSystem => diagnosticsSystem ?? (diagnosticsSystem = GetService<IMixedRealityDiagnosticsSystem>());

private static IMixedRealityFocusProvider focusProvider;

/// <summary>
/// Cached reference to the active instance of the focus provider.
/// If system is destroyed, reference will be invalid. Please use ResetCacheReferences()
/// </summary>
public static IMixedRealityFocusProvider FocusProvider => GetService<IMixedRealityFocusProvider>();
public static IMixedRealityFocusProvider FocusProvider => focusProvider ?? (focusProvider = GetService<IMixedRealityFocusProvider>());

private static IMixedRealityInputSystem inputSystem;

/// <summary>
/// Cached reference to the active instance of the input system.
/// If system is destroyed, reference will be invalid. Please use ResetCacheReferences()
/// </summary>
public static IMixedRealityInputSystem InputSystem => GetService<IMixedRealityInputSystem>();
public static IMixedRealityInputSystem InputSystem => inputSystem ?? (inputSystem = GetService<IMixedRealityInputSystem>());

private static IMixedRealityRaycastProvider raycastProvider;

/// <summary>
/// Cached reference to the active instance of the raycast provider.
/// If system is destroyed, reference will be invalid. Please use ResetCacheReferences()
/// </summary>
public static IMixedRealityRaycastProvider RaycastProvider => GetService<IMixedRealityRaycastProvider>();
public static IMixedRealityRaycastProvider RaycastProvider => raycastProvider ?? (raycastProvider = GetService<IMixedRealityRaycastProvider>());

private static IMixedRealitySceneSystem sceneSystem;

/// <summary>
/// Cached reference to the active instance of the scene system.
/// If system is destroyed, reference will be invalid. Please use ResetCacheReferences()
/// </summary>
public static IMixedRealitySceneSystem SceneSystem => GetService<IMixedRealitySceneSystem>();
public static IMixedRealitySceneSystem SceneSystem => sceneSystem ?? (sceneSystem = GetService<IMixedRealitySceneSystem>());

private static IMixedRealitySpatialAwarenessSystem spatialAwarenessSystem;

/// <summary>
/// Cached reference to the active instance of the spatial awareness system.
/// If system is destroyed, reference will be invalid. Please use ResetCacheReferences()
/// </summary>
public static IMixedRealitySpatialAwarenessSystem SpatialAwarenessSystem => GetService<IMixedRealitySpatialAwarenessSystem>();
public static IMixedRealitySpatialAwarenessSystem SpatialAwarenessSystem => spatialAwarenessSystem ?? (spatialAwarenessSystem = GetService<IMixedRealitySpatialAwarenessSystem>());

private static IMixedRealityTeleportSystem teleportSystem;

/// <summary>
/// Cached reference to the active instance of the teleport system.
/// If system is destroyed, reference will be invalid. Please use ResetCacheReferences()
/// </summary>
public static IMixedRealityTeleportSystem TeleportSystem => GetService<IMixedRealityTeleportSystem>();
public static IMixedRealityTeleportSystem TeleportSystem => teleportSystem ?? (teleportSystem = GetService<IMixedRealityTeleportSystem>());

/// <summary>
/// Resets all cached system references to null
/// </summary>
public static void ResetCacheReferences() => serviceCache.Clear();
public static void ResetCacheReferences()
{
serviceCache.Clear();
boundarySystem = null;
cameraSystem = null;
diagnosticsSystem = null;
focusProvider = null;
inputSystem = null;
raycastProvider = null;
sceneSystem = null;
spatialAwarenessSystem = null;
teleportSystem = null;
}

/// <summary>
/// Clears the cache of the reference with key of given type if present and applicable
Expand All @@ -91,6 +121,7 @@ public static bool ResetCacheReference(Type serviceType)
if (serviceCache.ContainsKey(serviceType))
{
serviceCache.Remove(serviceType);
ResetCacheReferenceFromType(serviceType);
return true;
}
}
Expand All @@ -102,6 +133,46 @@ public static bool ResetCacheReference(Type serviceType)
return false;
}

private static void ResetCacheReferenceFromType(Type serviceType)
{
if (typeof(IMixedRealityBoundarySystem).IsAssignableFrom(serviceType))
{
boundarySystem = null;
}
if (typeof(IMixedRealityCameraSystem).IsAssignableFrom(serviceType))
{
cameraSystem = null;
}
if (typeof(IMixedRealityDiagnosticsSystem).IsAssignableFrom(serviceType))
{
diagnosticsSystem = null;
}
if (typeof(IMixedRealityFocusProvider).IsAssignableFrom(serviceType))
{
focusProvider = null;
}
if (typeof(IMixedRealityInputSystem).IsAssignableFrom(serviceType))
{
inputSystem = null;
}
if (typeof(IMixedRealityRaycastProvider).IsAssignableFrom(serviceType))
{
raycastProvider = null;
}
if (typeof(IMixedRealitySceneSystem).IsAssignableFrom(serviceType))
{
sceneSystem = null;
}
if (typeof(IMixedRealitySpatialAwarenessSystem).IsAssignableFrom(serviceType))
{
sceneSystem = null;
}
if (typeof(IMixedRealityTeleportSystem).IsAssignableFrom(serviceType))
{
teleportSystem = null;
}
}

/// <summary>
/// Gets first matching <see cref="Microsoft.MixedReality.Toolkit.Input.IMixedRealityInputDeviceManager"/> or extension thereof for CoreServices.InputSystem
/// </summary>
Expand Down Expand Up @@ -147,7 +218,7 @@ public static T GetDataProvider<T>(IMixedRealityService service) where T : IMixe
// We do not want to keep a service around so use WeakReference
private static readonly Dictionary<Type, WeakReference<IMixedRealityService>> serviceCache = new Dictionary<Type, WeakReference<IMixedRealityService>>();

private static T GetService<T>() where T : IMixedRealityService
private static T GetService<T>() where T : IMixedRealityService
{
Type serviceType = typeof(T);

Expand Down Expand Up @@ -176,4 +247,4 @@ private static T GetService<T>() where T : IMixedRealityService
return service;
}
}
}
}
41 changes: 41 additions & 0 deletions SECURITY.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
<!-- BEGIN MICROSOFT SECURITY.MD V0.0.7 BLOCK -->

## Security

Microsoft takes the security of our software products and services seriously, which includes all source code repositories managed through our GitHub organizations, which include [Microsoft](https://github.com/Microsoft), [Azure](https://github.com/Azure), [DotNet](https://github.com/dotnet), [AspNet](https://github.com/aspnet), [Xamarin](https://github.com/xamarin), and [our GitHub organizations](https://opensource.microsoft.com/).

If you believe you have found a security vulnerability in any Microsoft-owned repository that meets [Microsoft's definition of a security vulnerability](https://aka.ms/opensource/security/definition), please report it to us as described below.

## Reporting Security Issues

**Please do not report security vulnerabilities through public GitHub issues.**

Instead, please report them to the Microsoft Security Response Center (MSRC) at [https://msrc.microsoft.com/create-report](https://aka.ms/opensource/security/create-report).

If you prefer to submit without logging in, send email to [[email protected]](mailto:[email protected]). If possible, encrypt your message with our PGP key; please download it from the [Microsoft Security Response Center PGP Key page](https://aka.ms/opensource/security/pgpkey).

You should receive a response within 24 hours. If for some reason you do not, please follow up via email to ensure we received your original message. Additional information can be found at [microsoft.com/msrc](https://aka.ms/opensource/security/msrc).

Please include the requested information listed below (as much as you can provide) to help us better understand the nature and scope of the possible issue:

* Type of issue (e.g. buffer overflow, SQL injection, cross-site scripting, etc.)
* Full paths of source file(s) related to the manifestation of the issue
* The location of the affected source code (tag/branch/commit or direct URL)
* Any special configuration required to reproduce the issue
* Step-by-step instructions to reproduce the issue
* Proof-of-concept or exploit code (if possible)
* Impact of the issue, including how an attacker might exploit the issue

This information will help us triage your report more quickly.

If you are reporting for a bug bounty, more complete reports can contribute to a higher bounty award. Please visit our [Microsoft Bug Bounty Program](https://aka.ms/opensource/security/bounty) page for more details about our active programs.

## Preferred Languages

We prefer all communications to be in English.

## Policy

Microsoft follows the principle of [Coordinated Vulnerability Disclosure](https://aka.ms/opensource/security/cvd).

<!-- END MICROSOFT SECURITY.MD BLOCK -->