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

Add wcfcore perf testing and deploy service with corewcf #4884

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Add wcfcore perf testing and deploy service with corewcf #4884

wants to merge 12 commits into from

Conversation

ZhaodongTian
Copy link
Contributor

Add perf testing with crank and deploy new service with corewcf. @HongGit @mconnew

@HongGit
Copy link
Contributor

HongGit commented Aug 29, 2022

The WCFCorePerf and WCFCorePerfService is duplicate with the desktop version. What about rename WCFCorePerfService to CoreWCFService, and WCFCorePerf to WCFCorePerfClientUsingCoreWCFService (you could omit Service)?

@ZhaodongTian
Copy link
Contributor Author

The WCFCorePerf and WCFCorePerfService is duplicate with the desktop version. What about rename WCFCorePerfService to CoreWCFService, and WCFCorePerf to WCFCorePerfClientUsingCoreWCFService (you could omit Service)?
I have renamed the project name

@ZhaodongTian ZhaodongTian reopened this Aug 31, 2022
@mconnew
Copy link
Member

mconnew commented Sep 1, 2022

This PR looks like it has a lot of other unrelated changes included in it that were already merged. It's making it really difficult to review. Can you rebase to main?

@ZhaodongTian
Copy link
Contributor Author

This PR looks like it has a lot of other unrelated changes included in it that were already merged. It's making it really difficult to review. Can you rebase to main?

I have rebased main and do a force push

HongGit
HongGit reviewed Oct 18, 2022
View reviewed changes
src/System.Private.ServiceModel/tests/Benchmarks/WCFCorePerf/benchmarks.wcfcore.Perf.yml Outdated
@@ -2,7 +2,7 @@ jobs:
WCFCorePerf:
source:
repository: https://github.com/dotnet/wcf.git
branchOrCommit: master
branchOrCommit: main
Copy link
Contributor

Choose a reason for hiding this comment

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

These two changes aren't related with using CoreWCF as server, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These two changes aren't related with using CoreWCF as server, correct?

You are right, that means the client code will pull from main branch

mconnew
mconnew reviewed Oct 21, 2022
View reviewed changes
...eModel/tests/Benchmarks/PerfUsingCoreWCFService/CoreWCFPerfService/CoreWCFPerfService.csproj
@@ -0,0 +1,15 @@
<Project Sdk="Microsoft.NET.Sdk">

Copy link
Member

Choose a reason for hiding this comment

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

Use the Sdk ""Microsoft.NET.Sdk.Web" and remove the package reference to Microsoft.AspNetCore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If change to "Microsoft.NET.Sdk.Web", visualstudio will throw IndexOutOfRangeException exception

mconnew
mconnew reviewed Oct 21, 2022
View reviewed changes
....Private.ServiceModel/tests/Benchmarks/PerfUsingCoreWCFService/CoreWCFPerfService/Program.cs
static void Main(string[] args)
{
string filePath = Path.Combine(Environment.CurrentDirectory, "WCFCorePerfService.exe");
Console.WriteLine(filePath);
Copy link
Member

Choose a reason for hiding this comment

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

I think a better option is Assembly.GetEntryAssembly().Location as the exe doesn't need to be launched from the current directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Assembly.GetEntryAssembly().Location will return the full path of coreWCFPerfService.dll. but I need to get the full path of exe and add into firewall

mconnew
mconnew reviewed Oct 22, 2022
View reviewed changes
....Private.ServiceModel/tests/Benchmarks/PerfUsingCoreWCFService/CoreWCFPerfService/Program.cs Outdated
string filePath = Path.Combine(Environment.CurrentDirectory, "WCFCorePerfService.exe");
Console.WriteLine(filePath);
string command = $" advfirewall firewall add rule name=\"WCFCorePerfService\" dir=in protocol=any action=allow program=\"{filePath}\" enable=yes";
ExecuteCommand(command, Environment.CurrentDirectory, TimeSpan.FromSeconds(20));
Copy link
Member

Choose a reason for hiding this comment

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

A better option is to directly modify the firewall. You can find some old code which does this here. It's basically a helper class which uses the COM component for programming the firewall and includes automatic cleanup of process exit. I believe this will work now as there's COM support in recent versions of .NET.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have use COM to add rules in the firewall. but this only support in windows OS

mconnew
mconnew reviewed Oct 22, 2022
View reviewed changes
...m.Private.ServiceModel/tests/Benchmarks/PerfUsingCoreWCFService/WCFCorePerfClient/Program.cs

BenchmarksEventSource.Measure("wcfcoreperf/requests", request);
BenchmarksEventSource.Measure("wcfcoreperf/rps/max", request / test._paramPerfMeasurementDuration.TotalSeconds);
break;
Copy link
Member

Choose a reason for hiding this comment

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

The value you are dividing by is the wrong one. The total time taken will always be more than the specified duration time by an average of half the average request time. You need to measure the actual duration taken to complete the while loop and divide by that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used EndTime-BeginTime , and found the value is equal to the specified duration. Maybe something wrong in the code

@mconnew
Copy link
Member

mconnew commented Oct 22, 2022 

        string filePath = Path.Combine(Environment.CurrentDirectory, "WcfCorePerfCrankService.exe");

My understanding was we weren't going to be downloading anything any more and would always compile the perf crank service fresh each time.

Refers to: src/System.Private.ServiceModel/tests/Benchmarks/PerfUsingNetfxWCFService/NetfxWCFPerfService/Program.cs:35 in c7d5bf1. [](commit_id = c7d5bf1, deletion_comment = False)

@ZhaodongTian
Copy link
Contributor Author 

        string filePath = Path.Combine(Environment.CurrentDirectory, "WcfCorePerfCrankService.exe");

My understanding was we weren't going to be downloading anything any more and would always compile the perf crank service fresh each time.

Refers to: src/System.Private.ServiceModel/tests/Benchmarks/PerfUsingNetfxWCFService/NetfxWCFPerfService/Program.cs:35 in c7d5bf1. [](commit_id = c7d5bf1, deletion_comment = False)

@mconnew Crank only support .netcore and cannot build projects which target with .netframework. so I use download way to start service. do you know other way to host .netframewrok service?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HongGit HongGit HongGit left review comments

@mconnew mconnew mconnew left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ZhaodongTian @HongGit @mconnew