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.

Sign up for GitHub

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

Jump to bottom

Using FastExpressionCompiler to compile the pipeline expression tree #5071

Merged
merged 2 commits into from
Aug 13, 2018
Merged

Using FastExpressionCompiler to compile the pipeline expression tree #5071

merged 2 commits into from
Aug 13, 2018

Conversation

danielmarbach
Copy link
Contributor

Would address #5070

@danielmarbach danielmarbach requested a review from a team November 13, 2017 12:04
danielmarbach
danielmarbach commented Nov 13, 2017
View reviewed changes
...rviceBus.Core.Tests/ApprovalFiles/PipelineTests.ShouldCreateCachedExecutionPlan.approved.txt
@@ -3,4 +3,4 @@
context2 => value(NServiceBus.Core.Tests.Pipeline.PipelineTests+Stage1).Invoke(context2, value(System.Func`2[NServiceBus.Pipeline.IIncomingLogicalMessageContext,System.Threading.Tasks.Task])),
context3 => value(NServiceBus.Core.Tests.Pipeline.PipelineTests+Behavior2).Invoke(context3, value(System.Func`2[NServiceBus.Pipeline.IIncomingLogicalMessageContext,System.Threading.Tasks.Task])),
context4 => value(NServiceBus.Core.Tests.Pipeline.PipelineTests+Stage2).Invoke(context4, value(System.Func`2[NServiceBus.Pipeline.IDispatchContext,System.Threading.Tasks.Task])),
context5 => value(NServiceBus.Core.Tests.Pipeline.PipelineTests+Terminator).Invoke(context5, value(System.Func`2[NServiceBus.Pipeline.PipelineTerminator`1+ITerminatingContext[NServiceBus.Pipeline.IDispatchContext],System.Threading.Tasks.Task])),
context5 => value(NServiceBus.Core.Tests.Pipeline.PipelineTests+Terminator).Invoke(context5, value(System.Func`2[NServiceBus.Pipeline.PipelineTerminator`1+ITerminatingContext[NServiceBus.Pipeline.IDispatchContext],System.Threading.Tasks.Task`1[System.Int32]])),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change should not be relevant since Task<TResult> is always assignable to Task.

src/NServiceBus.Core/Pipeline/FastExpressionCompiler.g.cs Outdated
*/

// ReSharper disable CoVariantArrayConversion
namespace FastExpressionCompiler
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added as source. V1.5

@dbelcham
Copy link
Contributor

@danielmarbach we talked about this in our sync and we like the idea. There's a PlatDev issue to drive out some of the questions that we have. This is likely going to be something that gets included in v7.1 or later. We don't see a reason that it would have to wait until v8 though.

@andreasohlund andreasohlund changed the title Using FastExpressionCompiler to compile the pipeline expression tree [WIP]Using FastExpressionCompiler to compile the pipeline expression tree Nov 29, 2017
@andreasohlund
Copy link
Member

Discussed on the core sync and marked as WIP until we have the results from the performance tests available

@danielmarbach
Copy link
Contributor Author

@andreasohlund what are the expectations from these tests?

@andreasohlund
Copy link
Member

andreasohlund commented Nov 30, 2017 •
edited
Loading

We just figured that we wanted to know how much faster the new pipeline is since there is no rush with this one? (will go into 7.1)

@andreasohlund andreasohlund added this to the 7.1.0 milestone Mar 28, 2018
@andreasohlund andreasohlund removed this from the 7.1.0 milestone Apr 18, 2018
@danielmarbach
Copy link
Contributor Author

Rebased and added FastExpressionCompiler v1.8

@timbussmann
Copy link
Contributor

@danielmarbach seems like the added sources add quite a lot of public APIs. It doesn't seem that we want those classes to be exposed to users?

I guess we can just make all the classes private?

@danielmarbach
Copy link
Contributor Author

I did that for the main class but forgot to redo it for the others when I updated. Will fix

@danielmarbach
Copy link
Contributor Author

Done. Sorry for the inconvenience

@timbussmann
Copy link
Contributor

it's not clearly visible what version of FastExpressionCompiler we're using here. Should we check for updates and also make a note which version was used?

Also, we should probably add it to this list: https://github.com/Particular/NServiceBus#licensing ?

@danielmarbach
Copy link
Contributor Author

@timbussmann have a look. I added a multiline comment with the version and an additional hint was was changed

@danielmarbach danielmarbach changed the title [WIP]Using FastExpressionCompiler to compile the pipeline expression tree Using FastExpressionCompiler to compile the pipeline expression tree Aug 13, 2018
timbussmann
timbussmann reviewed Aug 13, 2018
View reviewed changes
src/NServiceBus.Core/Pipeline/FastExpressionCompiler.g.cs Outdated
*/

/*
v1.10.1 from https://github.com/dadhi/FastExpressionCompiler
Copy link
Contributor

Choose a reason for hiding this comment

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

@danielmarbach latest version I see on that repo is 1.7.1. Where did you take that version number from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

Copy link
Contributor

Choose a reason for hiding this comment

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

@danielmarbach thanks, didn't see that. I was just looking at the releases and branches and didn't find this version there.

andreasohlund
andreasohlund previously approved these changes Aug 13, 2018
View reviewed changes
@bording
Copy link
Member

bording commented Aug 13, 2018

As @timbussmann mentioned, we should go ahead and update https://github.com/Particular/NServiceBus#licensing as part of this PR.

@danielmarbach
Copy link
Contributor Author

@bording done

@bording
Copy link
Member

bording commented Aug 13, 2018

@danielmarbach One last thing. Since it doesn't look like the FastExpressionCompiler repo tags releases, can you include the commit hash of the file as well so we can correlate back to a specific version?

@danielmarbach
Copy link
Contributor Author

@bording done

@timbussmann timbussmann added this to the 7.1.0 milestone Aug 13, 2018
@timbussmann timbussmann merged commit 090b796 into develop Aug 13, 2018
@timbussmann timbussmann deleted the fast-expression branch August 13, 2018 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bording bording bording approved these changes

@timbussmann timbussmann timbussmann approved these changes

@andreasohlund andreasohlund andreasohlund left review comments

Assignees

@timbussmann timbussmann

Labels
None yet
Projects
None yet
Milestone
7.1.3
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@danielmarbach @dbelcham @andreasohlund @timbussmann @bording