Skip to content

Removes the memory alloc of FocusProvider::GetPointers#8118

Closed
wiwei wants to merge 1 commit into
microsoft:mrtk_developmentfrom
wiwei:issue8109
Closed

Removes the memory alloc of FocusProvider::GetPointers#8118
wiwei wants to merge 1 commit into
microsoft:mrtk_developmentfrom
wiwei:issue8109

Conversation

@wiwei

@wiwei wiwei commented Jun 29, 2020

Copy link
Copy Markdown
Contributor

Overview

GetPointers currently returns an IEnumerable backed by a List, which allocs memory for each call to it. While we could have 'saved' on this by caching a result for a particular type T on a per-Update loop basis, this would also end up making things more complex than necessary (i.e. saving cached state, clearing it, making it possible for us to hit bugs there).

Since the function is already an IEnumerable, we can just use yield return typePointer to avoid allocing a return structure (thanks @Alexees for the solution here)

We use this in a bunch of non-test and test locations, so this is verified just with existing tests that pass.

Changes

@keveleigh

Copy link
Copy Markdown
Contributor

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 2 pipeline(s).

if (typePointer != null)
{
typePointers.Add(typePointer);
yield return typePointer;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

TIL c# has this kind of lazy evaluation

so cool

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@RogPodge from C# 7 you can even do this:

/// <inheritdoc />
        public IEnumerable<T> GetPointers<T>() where T : class, IMixedRealityPointer
        {
            foreach (var pointer in pointers.Values)
            {
                if (pointer.Pointer as T typePointer)
                {
                    yield return typePointer;
                }
            }
        }

@wiwei

wiwei commented Jul 14, 2020

Copy link
Copy Markdown
Contributor Author

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 2 pipeline(s).

@keveleigh

Copy link
Copy Markdown
Contributor

This is an older article (2016) and I've been meaning to retest their method, but it seems like this change might trade garbage for a perf cost: https://jacksondunstan.com/articles/3598

@david-c-kline

Copy link
Copy Markdown

seems like this change might trade garbage for a perf cost

Garbage will eventually result in a performance cost, but not as predictable. This would be good to test in an app with a typical
resource load.

@wiwei

wiwei commented Aug 24, 2020

Copy link
Copy Markdown
Contributor Author

Unfortunately I haven't had time to do the profiling for this (given @keveleigh 's previous comment - I'll close this out until then)

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.

FocusProvider.GetPointers<T> generates gargabe

5 participants