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

in parameter #316

Closed
sebastienros opened this issue Jan 6, 2022 · 13 comments
Closed

in parameter #316

sebastienros opened this issue Jan 6, 2022 · 13 comments
Assignees
Labels
Milestone

Comments

@sebastienros
Copy link
Contributor

On net6.0 only, with this class

    public class ParseException : Exception
    {
        public ParseException(string message, in TextPosition position) : base(message)
        {
            Position = position;
        }

        public TextPosition Position { get; set; }
    }

Try to create an instance like this:

Expression.Throw(Expression.New(typeof(ParseException).GetConstructors().First(), new [] { Expression.Constant("Message"), context.Position() } ))

Fails with a message similar to "invalid CLR ..." when invoking the lambda. Works fine when I remove the in marker in the arguments list. Works fine on netcoreapp3.1 and net5.0 in my tests.

@sebastienros
Copy link
Contributor Author

Apart from that, some feedback, got some relatively small expressions compiled from

|                         Method |       Mean |    Error |  StdDev |  Gen 0 |  Gen 1 | Allocated |
|------------------------------- |-----------:|---------:|--------:|-------:|-------:|----------:|
|      CreateCompiledSmallParser | 1,373.9 us | 31.08 us | 1.70 us | 7.8125 | 3.9063 |     50 KB |
| CreateCompiledExpressionParser |   200.1 us | 10.71 us | 0.59 us | 1.4648 | 0.7324 |     10 KB |

to

|                         Method |     Mean |     Error |   StdDev |  Gen 0 |  Gen 1 |  Gen 2 | Allocated |
|------------------------------- |---------:|----------:|---------:|-------:|-------:|-------:|----------:|
|      CreateCompiledSmallParser | 75.44 us | 24.697 us | 1.354 us | 5.3711 | 2.6855 | 0.1221 |     33 KB |
| CreateCompiledExpressionParser | 15.45 us |  1.171 us | 0.064 us | 0.8850 | 0.4272 |      - |      6 KB |

@dadhi
Copy link
Owner

dadhi commented Jan 6, 2022

First probably is an issue but the full test will help or better the PR with test. See the IssueTests project for examples.

Regarding the benchmarks I need the benchmark code to interpret them. I dont see what are you testing and how.

@sebastienros
Copy link
Contributor Author

Good point about the unit tests, I will create one. Tried my luck in case you already knew the limitation with the in modifier.

The benchmarks are just a celebratory comment to show how much improvement I am getting with FEC, just replacing a Compile() with CompileFast() when used in https://github.com/sebastienros/parlot

@dadhi
Copy link
Owner

dadhi commented Jan 6, 2022

Tried my luck in case you already knew the limitation with the in modifier.

Got it. There were some bugs reported before with in but those were fixed.

The benchmarks are just a celebratory comment

Great. I am usually adding the ratio and the fec/fast words into the benchmarks to get the results by glance. Here I got it wrong :-)

@dadhi dadhi self-assigned this Jan 31, 2022
@dadhi dadhi added this to the v3.2.2 milestone Jan 31, 2022
@dadhi dadhi added the bug label Jan 31, 2022
dadhi added a commit that referenced this issue Feb 1, 2022
@dadhi dadhi closed this as completed in d8899c7 Feb 1, 2022
@dadhi
Copy link
Owner

dadhi commented Feb 1, 2022

@sebastienros
I have added two tests based on your problem description.
But I did not get the "Invalid CLR..." exception, instead I get the NRE.
So I have fixed those in FEC.

If your case is different, please provide the test or PR (in the same file) and reopen the issue.

@sebastienros
Copy link
Contributor Author

I assume you don't publish your master branch on myget (for instance) automatically to test before it's released?

@dadhi
Copy link
Owner

dadhi commented Feb 1, 2022

No. Mmm.. I will check some options Tomorrow.

@dadhi
Copy link
Owner

dadhi commented Feb 2, 2022

@sebastienros v3.2.2 is on NuGet

@sebastienros
Copy link
Contributor Author

Saw that thank you, and was looking for a sponsor page on GH :/ doesn't work in Belarus?

@dadhi
Copy link
Owner

dadhi commented Feb 2, 2022

Honestly did not check. I am in Spain now.. so maybe it works. Will figure it out :)

@sebastienros
Copy link
Contributor Author

It would definitely work, should be super easy to setup. At least even if you get a cerbeza worth of it you should do it.

@dadhi
Copy link
Owner

dadhi commented Mar 2, 2022

@sebastienros Hi, I have setup the Sponsors. GitHub was taking its time to approve, otherwise yes, it was rather simple procedure.

@dadhi
Copy link
Owner

dadhi commented Mar 2, 2022

@sebastienros OMG thanks! :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants