[DO-NOT-MERGE] Request for comments by redknightlois · Pull Request #3808 · TechEmpower/FrameworkBenchmarks

redknightlois · 2018-05-31T17:05:27Z

Implementation of the full test suite on both MVC and Middleware configurations.
Docker image for a database server running on Windows with an embedded developer license (supports up to 9 cores with 36Gb of memory).

cc @bhauer

Still missing:

Setting up the docker instances for RavenDB on Linux (toolset directory)
Run TechEmpower in verify mode.
cc @aviviadi @ayende @gregolsky

@aviviadi

…igurations. Docker image for database server running on Windows. Still missing: - Setting up the docker instances for RavenDB on Linux (toolset directory) - Run TechEmpower in verify mode. cc @aviviadi @ayende @gregolsky

msmith-techempower · 2018-05-31T17:10:13Z

I do not see the Dockerfile for the database server mentioned in the original post. What am I missing?

redknightlois · 2018-05-31T17:11:53Z

Hi Mike,

For now only running on windows:

https://github.com/Corvalius/FrameworkBenchmarks/blob/647f0db808a7b363ef2949254144054585c69d6c/toolset/databases/ravendb-win/raven-win.dockerfile

msmith-techempower · 2018-05-31T17:13:17Z

AHHHH, thank you. I am just blind as a bat >_<

sebastienros · 2018-05-31T17:40:14Z

frameworks/CSharp/aspnetcore/Benchmarks/Controllers/MultipleQueriesController.cs

+        public MultipleQueriesController()
+        { }
+
+        public MultipleQueriesController(RavenDb provider)


Can't you implement IDb and use the same pattern as the other calls?

Tried, but the model has to be different. RavenDB does not support integer keys among other small different things. We can build a dedicated controller if you are worried about the impact that extra instance variable may have on the other controllers.

Right, that would be better. Also I can measure the impact it has on other scenarios (it can't have any) and get preliminary results with our infrastructure so you can iterate more quickly, based on your branches.

@sebastienros Done.

ayende · 2018-05-31T18:52:19Z

frameworks/CSharp/aspnetcore/Benchmarks/Controllers/FortunesController.cs

+        public FortunesController()
+        { }
+
+        public FortunesController(RavenDb provider)


Shouldn't this be used via HttpContext.RequestServices.GetRequiredService, like the others?

Not really, unless we want to introduce a scope factory. That instead of creating the DocumentStore they would just OpenSession.

ayende · 2018-05-31T18:54:06Z

frameworks/CSharp/aspnetcore/Benchmarks/Data/RavenDb.cs

+            for (var i = 0; i < 10001; i++)
+                _identifiers[i] = $"{_random.Next(1, 10001)}";
+
+            // We need to use different databases because we id's are integer and RavenDB does not have tables, it has collections


Why not use a id prefix, instead?

ayende · 2018-05-31T18:56:18Z

frameworks/CSharp/aspnetcore/Benchmarks/Data/RavenDb.cs

+
+                // TODO: We need to have an static index and stream it completely. 
+                var query = session.Advanced
+                                   .AsyncRawQuery<FortuneRaven>("from Fortune order by Id");


I think you can drop the order by Id here, this is the natural sort order when querying by collection only.

I was under the impression that it would be lexicographic instead. Gonna check that.

It is lexical anyway

Yes, you are right there... I may be able to simplify it further

ayende · 2018-05-31T18:57:43Z

frameworks/CSharp/aspnetcore/Benchmarks/Middleware/MultipleUpdatesRavenMiddleware.cs

+            {
+                var count = MiddlewareHelpers.GetMultipleQueriesQueryCount(httpContext);
+
+                var rows = await _db.LoadMultipleUpdatesRows(count);


Is there specific format required? We can use the streaming option to pass the raw results directly to the client, bypassing any work on the server side.

Yes, format is specified. There are even ordering issues AFAIK for streaming. Multiple updates is actually a Bulk Insert, haven't code it like that yet, so no streaming.

Might be better to see if we can do that directly via a query and stream it, yen.

ayende · 2018-05-31T18:58:10Z

frameworks/CSharp/aspnetcore/Benchmarks/Middleware/SingleQueryRavenMiddleware.cs

+            {               
+                var row = await _db.LoadSingleQueryRow();
+
+                var result = JsonConvert.SerializeObject(row, _jsonSettings);


Same here, we can skip this cost by piping the results directly

redknightlois · 2018-06-04T18:46:34Z

Hi guys, anything that we should know about this?

bhauer · 2018-06-05T02:08:21Z

@redknightlois Sorry, we've been distracted trying to get Round 16 wrapped up. We'll try to follow up soon!

sebastienros · 2018-06-05T17:28:47Z

Could you provide a database server image that runs on Linux?

redknightlois · 2018-06-06T13:44:18Z

@sebastienros Done. Last commit would give you a linux dockerfile

michaelhixson · 2018-06-16T18:11:06Z

@redknightlois What's the state of this PR from your perspective? Are you looking for guidance from TechEmpower to get the toolset actually using the new database?

I have an open PR where I added a new database implementation (SQL Server on linux) to the toolset, where a framework is using the new database and passing tests. See: #3807

See especially the changes I had to make in the toolset/benchmark and toolset/utils directories. We would need similar changes here to make the toolset recognize RavenDB.

I also had to change our main root Dockerfile to make the necessary Python package available to the toolset so that it was able to talk to the new database. If you need to make changes in that file, which is the source for the techempower/tfb image, then Travis isn't going to be able to test it successfully because Travis pulls the image from here instead of building it: https://hub.docker.com/r/techempower/tfb/

You could rebuild the techempower/tfb image and test it locally though. That's what I did for my PR.

redknightlois · 2018-06-19T14:40:33Z

What's the state of this PR from your perspective? Are you looking for guidance from TechEmpower to get the toolset actually using the new database?

We are setting the groundwork to make it part of the TechEmpower framework. What I am looking for is guidance on both how to integrate everything and also on the benchmark specifically to know for sure if we are not tripping on something that could either give us an unfair advantage or disadvantage against the rest.

I will look into #3807 to replicate the changes that we would need for it. If I have any question I will ask here.

msmith-techempower · 2018-07-31T18:31:13Z

@michaelhixson Does this require a patch from us? I am a bit confused on the status of this pull request.

redknightlois · 2018-07-31T19:16:00Z

@msmith-techempower I didn't continue it yet. I expect to have what I need to change from #3807, in any way if you prefer, you can close the Request for comments PR and I will create a new one when we continue with this.

sebastienros reviewed May 31, 2018

View reviewed changes

Physically separate IDb controller from Raven controller

ae08cf6

redknightlois force-pushed the ravendb branch from 92dcf21 to ae08cf6 Compare May 31, 2018 18:04

ayende reviewed May 31, 2018

View reviewed changes

Using ValueTasks instead of Tasks to avoid the allocations

ad053b5

add ravendb linux image

df5d269

msmith-techempower added the Response needed! label Jul 31, 2018

NateBrady23 closed this Nov 30, 2018

Conversation

redknightlois commented May 31, 2018

Uh oh!

msmith-techempower commented May 31, 2018

Uh oh!

redknightlois commented May 31, 2018

Uh oh!

msmith-techempower commented May 31, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

redknightlois May 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

redknightlois commented Jun 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bhauer commented Jun 5, 2018

Uh oh!

sebastienros commented Jun 5, 2018

Uh oh!

redknightlois commented Jun 6, 2018

Uh oh!

michaelhixson commented Jun 16, 2018

Uh oh!

redknightlois commented Jun 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

msmith-techempower commented Jul 31, 2018

Uh oh!

redknightlois commented Jul 31, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

redknightlois May 31, 2018 •

edited

Loading

redknightlois commented Jun 4, 2018 •

edited

Loading

redknightlois commented Jun 19, 2018 •

edited

Loading