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

Parse expressions where possible to avoid penalties for .Compile() #4712

Merged

Conversation

to11mtm
Copy link
Member

@to11mtm to11mtm commented Jan 6, 2021

This PR changes ExpressionExtensions.GetArguments() to parse expressions manually and avoid .Compile() wherever possible. This improves the speed of Expression based Props by 8-10%.

Copy link
Member

@Aaronontheweb Aaronontheweb left a comment

Choose a reason for hiding this comment

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

LGTM, but need to approve API specs here since ExpressionExtensions is public @to11mtm

@Aaronontheweb
Copy link
Member

API approvals are still failing for some reason....

@Aaronontheweb Aaronontheweb added this to High priority in Akka.NET Sprint 2/8 - 2/22 Feb 3, 2021
@IgorFedchenko
Copy link
Contributor

When I am running this locally, I can see that for all approval specs this line is removed in .received file:

[assembly: System.Reflection.AssemblyMetadataAttribute("RepositoryUrl", "https://github.com/akkadotnet/akka.net")]

But seems like there is also some problem with reporting - this is what I get locally, and AzDo says the same:

System.NullReferenceException
Object reference not set to an instance of an object.
   at ApprovalTests.Reporters.MultiReporter.Report(String approved, String received)
   at ApprovalTests.Approvers.FileApprover.ReportFailure(IApprovalFailureReporter reporter)
   at ApprovalTests.Core.Approver.Verify(IApprovalApprover approver, IApprovalFailureReporter reporter)
   at ApprovalTests.Approvals.Verify(IApprovalApprover approver, IApprovalFailureReporter reporter)
   at ApprovalTests.Approvals.Verify(IApprovalWriter writer, IApprovalNamer namer, IApprovalFailureReporter reporter)
   at ApprovalTests.Approvals.Verify(IApprovalWriter writer)
   at ApprovalTests.Approvals.Verify(String text, Func`2 scrubber)
   at Akka.API.Tests.CoreAPISpec.ApproveCore()

This RepositoryUrl line was added in this commit, with adding .net5 build support from 4 Jan. I will try to make it pass locally and let you know what has to be fixed. Or will fix our CI in separate PR.

@IgorFedchenko IgorFedchenko self-assigned this Feb 8, 2021
@IgorFedchenko
Copy link
Contributor

Err, sorry, seems like this was something related to my local installations. After installing .NET 5 I do get RepositoryUrl entry generated.

@to11mtm You just need to update Akka.API.Tests/CoreAPISpec.ApproveCore.approved.txt to include this code:

public class ParseArgumentException : System.Exception
    {
        public ParseArgumentException(string message, System.Exception ex) { }
    }

since you have added new public exception class. Just run Akka.API.Tests and copy text from .received.txt to approved.txt.

And I will try to fix the problem why our approval spec is failing with NRE instead of reporting the real problem.

@IgorFedchenko
Copy link
Contributor

Ok, in Gitter we decided to use ArgumentException instead. So this is waiting for @to11mtm now.

P.S. I am fixing our approval specs failure reporting here: #4766

@IgorFedchenko
Copy link
Contributor

Seems like only racy specs are failing here now...

@Aaronontheweb Aaronontheweb enabled auto-merge (squash) February 22, 2021 20:06
@Aaronontheweb Aaronontheweb merged commit 17b9dee into akkadotnet:dev Feb 22, 2021
Akka.NET Sprint 2/8 - 2/22 automation moved this from High priority to Closed Feb 22, 2021
@Aaronontheweb Aaronontheweb added this to the 1.4.17 milestone Feb 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants