C# extractor: Improve performance by changing the caching by calumgrant · Pull Request #615 · github/codeql

calumgrant · 2018-12-04T18:03:29Z

Achieves a ~10% single-threaded speedup (77s -> 69s) by changing the caching to entity->label, instead of entity->id & id->label. This removes one cache lookup in the case of a cache hit, and bypasses the ID cache which can be a little slow.

hvitved

One question.

hvitved · 2018-12-05T08:50:58Z

-            Label existingLabel;
-            if (labelCache.TryGetValue(id, out existingLabel))
+            // Look up the Id in the idLabelCache
+            if (idLabelCache.TryGetValue(id, out label))


Why is idLabelCache needed? It appears to me that this is dead code, as entityLabelCache.TryGetValue() returns false iff idLabelCache.TryGetValue() returns false?

After thinking about this, I see that this is indeed not dead code, but can be hit when two different entities share the same ID, and therefor label. Could you put a comment saying something like that here?

In some ways, it's a bug for two entities to map to the same label, and it could be a performance win if we removed this step.

As far as I remember, this could happen because two underlying Roslyn symbols could represent the same thing, but I forgot under which circumstances.

I added some debugging to see where the conflicting labels occurred, which flushed out a few bugs that I've fixed. Overall, the new design gives around a 25% speedup excluding CIL (which doesn't use this code).

C#: Remove unnecessary code from Property.

hvitved

Sounds awesome with the improved performance. I have some suggestions for changes, as well as some questions.

hvitved · 2018-12-19T13:38:00Z

-            switch (methodDecl.MethodKind)
+            var methodKind = methodDecl.MethodKind;
+
+            if(methodKind == MethodKind.ExplicitInterfaceImplementation)


hvitved · 2018-12-19T13:41:08Z

+        /// <param name="factory">The entity factory.</param>
+        /// <param name="init">The initializer for the entity.</param>
+        /// <returns>The new/existing entity.</returns>
+        public Entity CreateEntity2<Type, Entity>(ICachedEntityFactory<Type, Entity> factory, Type type) where Entity : ICachedEntity


CreateEntity2 -> CreateMaybeNullEntity?

That's not quite the right name for it. It's also used when you want to cache using the entity as the key, e.g.

When init is null,

When Type is a tuple. In each case the tuple may be different, but you want to index only Item1 (e.g. GetAlreadyCreated)

When you want to use the same init to create different entities depending on Type. E.g. when you want either a Type or a TypeRef.

hvitved · 2018-12-19T13:42:30Z

-                return true;
+                var entity = factory.Create(this, type);
+
+                if(entityLabelCache.TryGetValue(entity, out var label))


hvitved · 2018-12-19T13:43:49Z

+        private Entity CreateNonNullEntity<Type, Entity>(ICachedEntityFactory<Type, Entity> factory, Type init) where Entity : ICachedEntity
+        {
+            if (objectEntityCache.TryGetValue(init, out var cached))
+                return (Entity)cached;


Is this cast needed?

Yes - it's cached as an ICachedEntity and could theoretically give an InvalidCastException if you try to create it with a different type next time.

Ah, I failed to see that Entity was a type parameter; misread it as IEntity...

hvitved · 2018-12-19T19:09:58Z

+                    entity.Label = label;
+                    return entity;
+                }
+                else


I prefer branches myself -- I moved the return out of the branches instead.

hvitved · 2018-12-19T19:12:11Z

        }

-        readonly Dictionary<IId, Label> labelCache = new Dictionary<IId, Label>();
+        readonly Dictionary<IId, ICachedEntity> idLabelCache = new Dictionary<IId, ICachedEntity>();


Add #if DEBUG_LABELS

hvitved · 2018-12-19T19:16:54Z

-        {
-            return factory.CreateEntity(cx, (t1, t2));
-        }
+            where Entity : ICachedEntity => factory.CreateEntity2(cx, (t1, t2));


Why not factory.CreateEntity -- (t1, t2) is never null? Same below

Following on from the comment above - you want to use Tuple.Item1 as the key (the other columns may be different), which is the behaviour of CreateEntity2.

hvitved · 2018-12-19T19:17:53Z


        bool NeedsPopulation { get; }
+
+        object UnderlyingObject { get; }


It was quite useful for debugging. I'm minded to keep it in if that's ok?

hvitved · 2018-12-19T19:17:59Z

            get; private set;
        }

+        object ICachedEntity.UnderlyingObject => symbol;


hvitved · 2018-12-19T19:18:40Z

+                    entityLabelCache[entity] = label;
+                    DefineLabel(label, id);
+                    if (entity.NeedsPopulation)
+                        Populate(null, entity);


Why always null argument, and not init as ISymbol?

Looks like a bug....

Add more tests for duplicated entities, and fix some duplicated entities. Update the TupleTypes output - some extraneous results gone so it's probably better.

pavgust · 2018-12-20T20:43:19Z

This pull request introduces 1 alert when merging d73b28e into a600353 - view on LGTM.com

new alerts:

1 for Redundant ToString() call

Comment posted by LGTM.com

apply the implicit-this patch to the remaining go code

calumgrant added the C# label Dec 4, 2018

calumgrant assigned hvitved Dec 4, 2018

calumgrant requested a review from a team as a code owner December 4, 2018 18:03

hvitved reviewed Dec 5, 2018

View reviewed changes

calumgrant added 4 commits December 19, 2018 10:40

C#: Improve performance by mapping directly from entities to labels.

a7cdf52

C#: Fix label conflicts.

88734f1

C#: Remove unnecessary code from Property.

C#: Add a new object->entity cache.

93ce34a

C#: Fix changed unit tests.

6a54a6d

calumgrant force-pushed the cs/extractor-caching branch from 71872a7 to 6a54a6d Compare December 19, 2018 11:03

hvitved requested changes Dec 19, 2018

View reviewed changes

C#: Address review comments.

d73b28e

Add more tests for duplicated entities, and fix some duplicated entities. Update the TupleTypes output - some extraneous results gone so it's probably better.

hvitved approved these changes Dec 20, 2018

View reviewed changes

hvitved merged commit 5478155 into github:master Dec 21, 2018

calumgrant mentioned this pull request Jan 4, 2019

C#: Fix assembly labels #725

Closed

kamarcum unassigned hvitved Apr 28, 2020

cklin pushed a commit that referenced this pull request May 23, 2022

Merge pull request #615 from erik-krogh/explicit-this

adbe198

apply the implicit-this patch to the remaining go code


		bool NeedsPopulation { get; }

		object UnderlyingObject { get; }

Conversation

calumgrant commented Dec 4, 2018

Uh oh!

hvitved left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hvitved left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pavgust commented Dec 20, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants