Remove unnecessary remove from ProjectRootElementCache

[Forgind]

It's fairly straightforward to demonstrate that if we reach this block, then it isn't in the _strongCache anyway, so this is entirely unnecessary work.

[ladipro]

Looks great! An idea for further optimization: The iteration pattern foreach (string projectPath in oldWeakCache.Keys) is pretty inefficient.

[ladipro]

[nit / pre-existing issue] What guarantees that this PRE was in the old strong cache and is actually being removed? It looks like we also get here if it was a regular weak-only not-explicitly-loaded entry.

[Forgind]

Good point. There are multiple ways we could go down this path and erroneously call the remove event. I wanted to make this an unambiguous no-op, so I stopped short of removing the next call, which might be legitimate. If I were to rework it to be more correct, I think this is best:

if (rootElement.IsExplicitlyLoaded)
{
    if (oldStrongCache.Contains(rootElement))
    {
        _strongCache.AddFirst(rootElement);
    }
    else
    {
        // Has already been removed. We can consider adding it to the strong cache here...but it probably isn't worth it
    }
}
else
{
    if (oldStrongCache.Contains(rootElement))
    {
        RaiseProjectRootElementRemovedFromStrongCache(rootElement);
    }
    else
    {
        // Element was not in the strong cache and should not be added to it --> no op
    }
}

or more simply

if (oldStrongCache.Contains(rootElement))
{
    if (rootElement.IsExplicitlyLoaded)
    {
        _strongCache.AddFirst(rootElement);
    }
    else
    {
        RaiseProjectRootElementRemovedFromStrongCache(rootElement);
    }
}

I think either is better from a correctness perspective, but it means we'd always be calling Contains, which is a bit slow.

[ladipro]

What if we make it iterate over the strong cache? Should be doable in one pass. And as a bonus, it would preserve the order, which the current implementation does not.

[Forgind]

The only issue I see with that is that we're also discarding implicit references in the weak cache in this pass, and since there can be ProjectRootElements in the weak cache but not the strong cache, we'd be deleting those from the weak cache whether they're explicitly loaded and not GC'd or not.

[Forgind]

The iteration pattern foreach (string projectPath in oldWeakCache.Keys) is pretty inefficient.

When I was working on the simpler loops PR, this one stuck out to me as one I wanted to optimize but was having trouble finding a way to make it better because I can't iterate over kvps in a weak cache. What do you suggest?

[Forgind]

Good point. There are multiple ways we could go down this path and erroneously call the remove event. I wanted to make this an unambiguous no-op, so I stopped short of removing the next call, which might be legitimate. If I were to rework it to be more correct, I think this is best:

if (rootElement.IsExplicitlyLoaded)
{
    if (oldStrongCache.Contains(rootElement))
    {
        _strongCache.AddFirst(rootElement);
    }
    else
    {
        // Has already been removed. We can consider adding it to the strong cache here...but it probably isn't worth it
    }
}
else
{
    if (oldStrongCache.Contains(rootElement))
    {
        RaiseProjectRootElementRemovedFromStrongCache(rootElement);
    }
    else
    {
        // Element was not in the strong cache and should not be added to it --> no op
    }
}

or more simply

if (oldStrongCache.Contains(rootElement))
{
    if (rootElement.IsExplicitlyLoaded)
    {
        _strongCache.AddFirst(rootElement);
    }
    else
    {
        RaiseProjectRootElementRemovedFromStrongCache(rootElement);
    }
}

I think either is better from a correctness perspective, but it means we'd always be calling Contains, which is a bit slow.

[ladipro]

The iteration pattern foreach (string projectPath in oldWeakCache.Keys) is pretty inefficient.

When I was working on the simpler loops PR, this one stuck out to me as one I wanted to optimize but was having trouble finding a way to make it better because I can't iterate over kvps in a weak cache. What do you suggest?

I was thinking about having WeakValueDictionary<K, V> implement IEnumerable<KeyValuePair<K, V>> like so:

        public IEnumerator<KeyValuePair<K, V>> GetEnumerator()
        {
            foreach (KeyValuePair<K, WeakReference<V>> kvp in _dictionary)
            {
                if (kvp.Value == null)
                {
                    yield return new KeyValuePair<K, V>(kvp.Key, null);
                }
                else if (kvp.Value.TryGetTarget(out V target))
                {
                    yield return new KeyValuePair<K, V>(kvp.Key, target);
                }
            }
        }

[Forgind]

The iteration pattern foreach (string projectPath in oldWeakCache.Keys) is pretty inefficient.

When I was working on the simpler loops PR, this one stuck out to me as one I wanted to optimize but was having trouble finding a way to make it better because I can't iterate over kvps in a weak cache. What do you suggest?

I was thinking about having WeakValueDictionary<K, V> implement IEnumerable<KeyValuePair<K, V>> like so:

        public IEnumerator<KeyValuePair<K, V>> GetEnumerator()
        {
            foreach (KeyValuePair<K, WeakReference<V>> kvp in _dictionary)
            {
                if (kvp.Value == null)
                {
                    yield return new KeyValuePair<K, V>(kvp.Key, null);
                }
                if (kvp.Value.TryGetTarget(out V target))
                {
                    yield return new KeyValuePair<K, V>(kvp.Key, target);
                }
            }
        }

I totally missed that WeakValueDictionary was something we owned! I'd be happy to implement that as part of this PR or as something separate!

[Forgind]

I implemented IEnumerable<K, V>. I'm unclear on the merit of returning (key, null) if the V was collected already, so I left that part out, but if there's a good reason for it, I'm happy to add it back. (In the use case in this PR, it just leads to extra checks on the other end.)

[ladipro]

I implemented IEnumerable<K, V>. I'm unclear on the merit of returning (key, null) if the V was collected already, so I left that part out, but if there's a good reason for it, I'm happy to add it back. (In the use case in this PR, it just leads to extra checks on the other end.)

(key, null) would be returned if the value stored in the dictionary was actually null (different from a originally non-null but now collected value - that should not be returned). Your implementation works for the use case of enumerating non-null values but it is not consistent with the rest of its API. It should enumerate all keys, for which TryGetValue returns true.