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

Why ToLambda<T> does not evaluate parameters dynamically #13

Open
lukas-ais opened this issue Sep 3, 2018 · 4 comments
Open

Why ToLambda<T> does not evaluate parameters dynamically #13

lukas-ais opened this issue Sep 3, 2018 · 4 comments

Comments

@lukas-ais
Copy link
Contributor

I have a problem similar to issue #6.
I have analyzed the code trying to solve the issue.

I'm wondering why the parameters are referenced as constant and not used dynamically.
Is there any use case to

  1. use parameters
  2. create a lambda
  3. call lambda function multiple times with same parameters?

If so, we need an option.
Otherwise I would like to rewrite the handling of parameters introduced with fixing #1.

@sklose
Copy link
Owner

sklose commented Sep 4, 2018

Hi lukas-ais,

I think there are a couple of different aspects to this:

  • the values in the parameters dictionary are objects and the compiler needs to know the exact type at compile time to created an optimized lambda ... that limits how dynamic that parameters dictionary can be. You could move all the type checking into the compiled lambda, but then you have a performance hit on every lambda call vs. once at compile time.
  • the compiled lambda exists independent of the Expression object, so using the parameters dictionary on the Expression object feels a little clunky design-wise ... there is no obvious link between the two left once the expression is compiled into a lambda
  • you might end up with memory leaks if you couple the compiled lambda to the parameters dictionary in the Expression object. AFAIK the CLR cannot garbage collect unused compiled lambda functions and if the lambda references the dictionary which is contained in the Expression then you have an entire object tree that can never be collected
  • there is already a way to use dynamic parameters with compiled expression (the expression context) ... please see the README for an example
  • if the expression context is not sufficient for your use case (e.g. you have a dynamic list of parameter names) then I think it could be useful to add support for dictionary access to the lambda compiler. Meaning the expression context could either be a dictionary or there could be a dictionary on the expression context and the lambda compiler can treat dictionary keys like properties. This will face the same issue as mentioned in point 1 though ... it will have runtime overhead on every single lambda evaluation since the type is not known in advance.

For example this ...

class ExpressionContext {
  public string Name { get; set; }
  public Nested Nested { get; set; }
}

class Nested {
  public string Value { get; set; }
}

... could be equivalent to:

class ExpressionContext {
  public string Name { get; set; }
  public Dictionary<string, object> Nested { get; set; }
}

or you could directly use a Dictionary<string, object> as the expression context.
I am happy to accept a PR for extending the expression context to support dictionaries if you feel like jumping in here :)

@lukas-ais
Copy link
Contributor Author

I already implemented the processing of the parameters within lambda.
The next days I'll send a pull request.
Meanwhile I found, that the extensibility (events EvaluateFunction and EvaluateParameter) are ignored in the current lambda handling. Therefore I will propose a solution too.

@kevinwong15
Copy link

Did the pull request for Dictionary<string, object> as the expression context go in?

@lukas-ais
Copy link
Contributor Author

Did the pull request for Dictionary<string, object> as the expression context go in?

I'm sorry, I didn't understand what you meant by that.
Could you please describe your use case?

You can also get the code or binary and just try it.

@sklose, how about my PR? I just fixed the merge conflicts.

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

Successfully merging a pull request may close this issue.

3 participants