Conversation
Agent-Logs-Url: https://github.com/dadhi/FastExpressionCompiler/sessions/631bfa1a-7ed7-4397-b2cb-ab11dfbcbdee Co-authored-by: dadhi <39516+dadhi@users.noreply.github.com>
Agent-Logs-Url: https://github.com/dadhi/FastExpressionCompiler/sessions/631bfa1a-7ed7-4397-b2cb-ab11dfbcbdee Co-authored-by: dadhi <39516+dadhi@users.noreply.github.com>
|
@copilot The conversation from/to LE is to be able to test correctness etc. and to have a fallback for now. The goal is to have FE as a primary build and compiled representation and superseef the LE with it. So, did you created the factory methods for building FE directly with the surface api matching LE? If not please do, because next next step would be integrating with compiler without too many '#if defs'. |
Agent-Logs-Url: https://github.com/dadhi/FastExpressionCompiler/sessions/15dd11ed-8bf0-4fd4-b514-3d2ff4e4e346 Co-authored-by: dadhi <39516+dadhi@users.noreply.github.com>
Agent-Logs-Url: https://github.com/dadhi/FastExpressionCompiler/sessions/15dd11ed-8bf0-4fd4-b514-3d2ff4e4e346 Co-authored-by: dadhi <39516+dadhi@users.noreply.github.com>
Added the direct FE builder surface on |
dadhi
left a comment
There was a problem hiding this comment.
Ci build is failing because of missing xml doc comments. Please add a minimal summary-focused comment explaining what and why. Be technical and point, avoid windy ramblings.
Agent-Logs-Url: https://github.com/dadhi/FastExpressionCompiler/sessions/0ccd5cdb-3248-46a4-b6df-54762eed9357 Co-authored-by: dadhi <39516+dadhi@users.noreply.github.com>
Added concise XML summaries to the new public flat-expression API and fixed the net472 test project reference needed by the dynamic binder test so the release build can complete. Addressed in cc19f39. |
| if (expressions == null || expressions.Length == 0) | ||
| throw new ArgumentException("Block should contain at least one expression.", nameof(expressions)); | ||
|
|
||
| var children = new List<int>(); |
There was a problem hiding this comment.
Avoid using list for creating a transient storage - use SmallListN or SmallMapN instead.
Goal is to make FE creation as fast and as gc free as possible. Apply in other places.
Look at LightExpression construction where I try to minimiza gc pressure.
We may use params ReadOnlySpan in .net 10 to minimize it further.
Avoid copying the big structs and using interim steuct data wrappers - prefer the diect style of manipulating the ExprNode. Users will know that the goal an extreme performance - so the responsibility is on their side. We may hrlp with Asserts, or making a right thing by construction but perf is the altimate goal.
There was a problem hiding this comment.
Reworked the transient child storage in FlatExpression to use stack-backed SmallList instead of List<int> across the direct builder, flattening builder, child cloning, and reader paths, and switched node creation to write through AddDefaultAndGetRef to avoid the extra ExprNode copy. Verified with dotnet build src/FastExpressionCompiler.LightExpression/FastExpressionCompiler.LightExpression.csproj -c Release and the net9 test runner. Addressed in 7958d82. No UI changes.
Agent-Logs-Url: https://github.com/dadhi/FastExpressionCompiler/sessions/a101d2c7-fa90-45b2-b621-f056a99693eb Co-authored-by: dadhi <39516+dadhi@users.noreply.github.com>
| private static System.Linq.Expressions.NewExpression CreateValueTypeNewExpression(Type type) => SysExpr.New(type); | ||
| } | ||
|
|
||
| private sealed class ParameterData |
There was a problem hiding this comment.
We should replace the class ...Data objects with storing the fields directly in the ExprNode.
Storing in the GC collected classes defeats the goal of FlatExpression - to be flat, cache-friendly, no chasing pointers, list of nodes.
For instance, for Parameter we can store Name in Obj; and the rest in the _data, same for label.
If the Expression type requires more than 1 object reference (optional comparison method) it should be stored in the child sub nodes.
Please adopt and remove all the Data classes below.
There was a problem hiding this comment.
Replaced the remaining ...Data wrapper payloads by storing scalar payload directly on ExprNode, moving single-reference payloads into Obj, and encoding the extra Dynamic reference as an internal child metadata node. Verified with the full release build and the net9 test runner. Addressed in 7f3a5b6. No UI changes.
| } | ||
|
|
||
| internal void SetNextIdx(int nextIdx) => | ||
| _data = (_data & ~(IndexMask << NextShift)) | ((ulong)(ushort)nextIdx << NextShift); |
There was a problem hiding this comment.
~(IndexMask << NextShift) and ~((IndexMask << CountShift) | IndexMask)
Can we just introduce derived nicely named constants for them and compress those calculations for SetNextIdx and SetChildInfo?
There was a problem hiding this comment.
Introduced named derived masks for the packed next/child bits and simplified SetNextIdx / SetChildInfo to use them directly. Addressed in 88fc048. No UI changes.
| private int AddFactoryExpressionNode(Type type, object obj, ExpressionType nodeType, byte flags, int child0, int child1) => | ||
| AddNode(type, obj, nodeType, ExprNodeKind.Expression, flags, CloneChild(child0), CloneChild(child1)); | ||
|
|
||
| private int AddFactoryExpressionNode(Type type, object obj, ExpressionType nodeType, byte flags, int child0, int child1, int child2) => |
There was a problem hiding this comment.
Let support up to 7 arguments to match LightExpression, plus minify the naming to avoid long winded signature - it is already clear that we are working with children/arguments/expressions/vairables/etc.
There was a problem hiding this comment.
Extended the compact fixed-arity expression helper path up to 7 children and shortened the helper child naming so the small-arity cases stay direct and closer to the LE style. Addressed in 88fc048. No UI changes.
| // Any leaf reused in more than one parent would have its intrusive sibling link (`NextIdx`) overwritten. | ||
| // Clone the leaf before linking so the direct builder API may safely reuse returned indexes for | ||
| // parameters, labels, constants, default values and parameterless `new` expressions. | ||
| private static bool ShouldCloneWhenLinking(in ExprNode node) => |
There was a problem hiding this comment.
Those checks that only operate on the one node state, can we move them into the Node struct itself and simplify? Do for the similar other situations as well for consistency.
There was a problem hiding this comment.
Moved the node-local predicates into ExprNode itself (Is, IsExpression, HasFlag, ShouldCloneWhenLinked) and updated the surrounding code to use them consistently. Addressed in 88fc048. No UI changes.
| return nodeIndex; | ||
| } | ||
|
|
||
| private static bool ShouldInlineConstant(object value, Type type) |
There was a problem hiding this comment.
Minify the check code into a single condition, avoid loc creep. Do the same for similar siutations/check.
Apply AggressiveInlining for such methods where it make sense.
There was a problem hiding this comment.
Compacted the small predicate/check helpers and added AggressiveInlining where it fits the hot tiny methods involved in the packed-node path. Addressed in 88fc048. No UI changes.
Agent-Logs-Url: https://github.com/dadhi/FastExpressionCompiler/sessions/d56dd094-3b5e-41e0-a3c3-80c72ae88fdd Co-authored-by: dadhi <39516+dadhi@users.noreply.github.com>
Yes — the direct FE factory surface matching the LE-style builder API is already in the branch on |
dadhi
left a comment
There was a problem hiding this comment.
Cool, next step I want a performance test to compare relative performance and memory allocation taking by creation of moderate real-world example, between LightExpression and FlatExpression. I will suggest to use latest CsCheck library (v4.6.2). - Later we may utilize for the expression fuzzy- and invariant-testing
Agent-Logs-Url: https://github.com/dadhi/FastExpressionCompiler/sessions/ed01d5a9-8337-4a93-a276-4c52a481fc65 Co-authored-by: dadhi <39516+dadhi@users.noreply.github.com>
Agent-Logs-Url: https://github.com/dadhi/FastExpressionCompiler/sessions/ed01d5a9-8337-4a93-a276-4c52a481fc65 Co-authored-by: dadhi <39516+dadhi@users.noreply.github.com>
Added |
dadhi
left a comment
There was a problem hiding this comment.
Let start the deterministic property testing to at least provide some guarantees for our very broad api surface. I have an idea to test first - build/genetate arbitraty expression with specified limits on tje depth and breadth via CsCheck, is ensure its validity by black listing some expressions in some contexts. Ley gen the same FE and LE expression, then convert FE to LE and compare them structurally - the structural comparison for LE and SE was just merged into master. If you cannot find how to that CsCheck let produce our own valid expression generator (using the ssme perf/simplicity principmes as the rest of the Fec).
Agent-Logs-Url: https://github.com/dadhi/FastExpressionCompiler/sessions/b03ffb96-068c-4255-bd58-3ca2f3ef1298 Co-authored-by: dadhi <39516+dadhi@users.noreply.github.com>
Agent-Logs-Url: https://github.com/dadhi/FastExpressionCompiler/sessions/b03ffb96-068c-4255-bd58-3ca2f3ef1298 Co-authored-by: dadhi <39516+dadhi@users.noreply.github.com>
Added the first deterministic property-style coverage pass in Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
dadhi
left a comment
There was a problem hiding this comment.
Add FlatExpression.cs to the src nudpecs.
Add tests to both .net472 the same as to .net test runner.
Ensure that it is included into the packages the same way as LightExpression.
It won't be integrated the same way in Fec and tests yet until we do this integration in the separate PR.
This change introduces a flat, intrusive-array expression representation for LightExpression-oriented workflows, with round-trip conversion back to
System.Linq.Expressionsand LightExpression. It also closes the remainingToLightExpressiongaps so the flat model can cover the expression surface area LE already supports today, adds a direct FE factory surface so flat expressions can be built without going through LE first, adds a focused benchmark to compare LightExpression and FlatExpression creation costs on the same moderate real-world shape, and starts deterministic property-style coverage for FE/LE structural round-trips on a bounded generated subset.Flat expression model
FastExpressionCompiler.FlatExpression.ExprTreeandExprNodeDirect FE builder API
ExprTreeConversion pipeline
System.Linq.Expressions.Expression -> ExprTreeFastExpressionCompiler.LightExpression.Expression -> ExprTreeExprTree -> System.Linq.Expressions.ExpressionExprTree -> FastExpressionCompiler.LightExpression.ExpressionLE compatibility coverage
ToLightExpressionto handle the remaining supported LE node kinds that were still missing:DynamicRuntimeVariablesDebugInfoBehavioral coverage
Dynamic,RuntimeVariables, andDebugInfoconversion pathsExprTreefactory APIPerformance coverage
LightExprVsFlatExpr_Create_ComplexExprto the existing benchmarks projectCreateComplexFlatExpressionhelper so the LE and FE benchmark shapes stay alignedDeterministic property coverage
LightExpressionTestsCsCheck4.6.2onnet8.0/net9.0to drive fixed-seed samplingmasterExample:
Type,MemberInfo,MethodInfo, binders), so this is not yet a fully serialization-ready wire formatFromLightExpressioncurrently flattens viaSystem.Linq.Expressionsrather than directly from LE nodesToCSharpString, orToExprStringchanges in this PR