Initial codegen node factory cleanup by lkts · Pull Request #6411 · dotnet/corert

lkts · 2018-10-01T19:41:36Z

This is a very harsh WIP, but i lack domain knowledge to proceed :)
I poked this class a bit and here are couple of observations/questions:

It is not very easy to stop deriving from NodeFactory.
There are some places where methods are called through base class: HeaderNode which derives from ObjectNode and other base infrastructure: CompilationBuilder and Compilation.
To split this class some approach is needed for initializing and passing around objects which currently are exposed as fields (f.e. "Import" ones are commonly used).
I hached this by passing whole factory to new extracted class but it is indeed a hack.
Also initialization process in AttachToDependencyGraph is complex and I am not sure if it can/should be split.
What exactly are "Token collection and lookup engine" and "R2R header table collection management." ? The only one method that mentions "token" seems unused.

Fixes https://github.com/dotnet/corert/issues/6274

cc @nattress

trylek · 2018-10-11T01:23:18Z

I believe this is a great step towards making the R2RCGNF more reasonable and less of a dumping ground for arbitrary code that didn't fit elsewhere. Let's hear what Simon has to say but to me your change looks good - I'm ready to approve it as soon as you remove the WIP prefix from the PR title. You will probably need to resolve some conflicts there - that's actually one more reason why it might be useful to just finish the review, merge the change in and possibly start another one if you see opportunities for additional cleanup; there's lots of traffic in this branch and maintaining an open PR for too long is often quite painful.

Thanks a lot Oleksandr for making these changes!

nattress · 2018-10-11T18:21:59Z

@@ -0,0 +1,39 @@
+using System;


Each source file needs to have the copyright header added. Could you please copy-paste from another file to top here.

nattress · 2018-10-11T18:30:13Z

+            return info.Constructor;
+        }
+
+        Dictionary<ILCompiler.ReadyToRunHelper, ISymbolNode> _helperCache = new Dictionary<ILCompiler.ReadyToRunHelper, ISymbolNode>();


Nit: Prepend with private

nattress · 2018-10-11T18:30:34Z

+            return ExternSymbol(helperId);
+        }
+
+        readonly Dictionary<MethodAndCallSite, ISymbolNode> _interfaceDispatchCells = new Dictionary<MethodAndCallSite, ISymbolNode>();


Nit: Prepend with private

nattress · 2018-10-11T18:30:41Z

+            return ReadyToRunHelper(helperId, entity, signatureContext);
+        }
+
+        readonly Dictionary<MethodDesc, ISortableSymbolNode> _genericDictionaryCache = new Dictionary<MethodDesc, ISortableSymbolNode>();


Nit: Prepend with private

nattress · 2018-10-11T18:30:49Z

+            return genericDictionary;
+        }
+
+        readonly Dictionary<TypeDesc, ISymbolNode> _constructedTypeSymbols = new Dictionary<TypeDesc, ISymbolNode>();


Nit: Prepend with private

nattress · 2018-10-11T20:47:52Z

Oleksandr this looks great - thanks for taking the time to organize this class. We are still discussing how we want this to look in the future but for now I'll gladly take this cleanup. I would leave things as they are in AttachToDependencyGraph in this PR.

For your question #3 above, could you point at where in the code you're referring?

lkts · 2018-10-12T19:28:48Z

Tomas, Simon, thank you for feedback. Merging changes one by one definitely is the right think to do. I had an idea to introduce something like CodegenContext which would be responsible for AttachToDependencyGraph and will be passed to factories but i didn`t look if it is possible.

For now I have done a pretty painful rebase, hope that everything is okay.

Regarding my third question - I was referring to the description of https://github.com/dotnet/corert/issues/6274 , third bullet point there.

lkts · 2018-10-12T19:36:17Z

@dotnet-bot test OSX10.12 Debug and CoreCLR tests

hm, how do i unWIP it?

trylek · 2018-10-12T20:25:19Z

Hmm, I hit that myself sometime ago, apparently there's some "smart" logic that WIP's your change whenever it finds WIP anywhere in the description, not just in the title. I'd try to edit the change description and leave out the reference to WIP; after that, you might need to retest the change to get rid of the dotnet bot still treating the change as WIP.

lkts · 2018-10-13T07:23:14Z

Turns out commit messages turn on the WIP (along with title and labels). I guess I should use only title for WIPs in future as it is the most confortable way.

Upd: not sure what`s up with WASM test.

trylek · 2018-10-13T08:23:40Z

Don't worry about WASM, it's been broken half of the time with my and Simon's changes too. We'll need to take a look at some point but it's not Pri# 0 right now.

trylek · 2018-10-11T01:13:12Z


        public new ReadyToRunCodegenNodeFactory NodeFactory { get; }
+
+        public ReadyToRunSymbolNodeFactory SymbolNodeFactory { get; }


I'm wondering whether it might be more descriptive to call the new class "ReadyToRunHelperFactory".

probably :)

trylek · 2018-10-11T01:13:46Z

            : base(dependencyGraph, nodeFactory, roots, ilProvider, debugInformationProvider, devirtualizationManager, logger)
        {
            NodeFactory = nodeFactory;
+            SymbolNodeFactory = new ReadyToRunSymbolNodeFactory(nodeFactory);


I don't think this is such a big hack. It will probably take several iterations to finish the transition, for now I believe this is just fine.

trylek · 2018-10-11T01:16:10Z

@@ -0,0 +1,39 @@
+using System;


Nit - I'm not sure how much value there is in separating out this tiny class considering there are ten others like this in the node / helper factory but I don't have a strong opinion on this; perhaps you're right that longer term we would be better off pulling all these helpers out to separate classes.

It is used in both factories, that`s why i extracted it.

* Initial ReadyToRunCodegenNodeFactory cleanup * Extract part of ReadyToRunCodegenNodeFactory to separate class * Fixes after rebase * Addressed code review feedback * Fixed rebase issue * Minor additional cleanup

lkts changed the title ~~Codegen node factory cleanup~~ WIP: Codegen node factory cleanup Oct 1, 2018

nattress reviewed Oct 11, 2018

View reviewed changes

lkts force-pushed the codegen-node-factory-cleanup branch from 8cd9a85 to 95d88dc Compare October 12, 2018 19:23

lkts changed the title ~~WIP: Codegen node factory cleanup~~ Initial codegen node factory cleanup Oct 12, 2018

cod7alex added 6 commits October 13, 2018 10:20

Initial ReadyToRunCodegenNodeFactory cleanup

2a37fdc

Extract part of ReadyToRunCodegenNodeFactory to separate class

c3bb2b5

Fixes after rebase

ec77308

Addressed code review feedback

0bd6857

Fixed rebase issue

ce30879

Minor additional cleanup

1e3ae4c

lkts force-pushed the codegen-node-factory-cleanup branch from 95d88dc to 1e3ae4c Compare October 13, 2018 07:20

trylek approved these changes Oct 13, 2018

View reviewed changes

trylek merged commit e4e11db into dotnet:r2r Oct 13, 2018

lkts deleted the codegen-node-factory-cleanup branch October 13, 2018 08:40


		public new ReadyToRunCodegenNodeFactory NodeFactory { get; }

		public ReadyToRunSymbolNodeFactory SymbolNodeFactory { get; }

Conversation

lkts commented Oct 1, 2018

Uh oh!

trylek commented Oct 11, 2018

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!

nattress commented Oct 11, 2018

Uh oh!

lkts commented Oct 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lkts commented Oct 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

trylek commented Oct 12, 2018

Uh oh!

lkts commented Oct 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

trylek commented Oct 13, 2018

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lkts commented Oct 12, 2018 •

edited

Loading

lkts commented Oct 12, 2018 •

edited

Loading

lkts commented Oct 13, 2018 •

edited

Loading