Skip to content

[mORMot] speed up pipelining; speed up docker build #8057

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

Merged

Conversation

pavelmash
Copy link
Contributor

mORMot using a modified libpq that allows to send the PipelineSync command without flushing prev. commands to socket

…command without flushing prev. commands to socket
@pavelmash pavelmash changed the title [mORMot] send all pipelining comands by one socket send syscall [mORMot] speen up pipelining; speed up docker build Mar 21, 2023
@pavelmash pavelmash changed the title [mORMot] speen up pipelining; speed up docker build [mORMot] speed up pipelining; speed up docker build Mar 21, 2023
@@ -193,6 +193,7 @@ constructor TRawAsyncServer.Create(
hsoIncludeDateHeader // required by TPW General Test Requirements #5
] + flags);
fHttpServer.HttpQueueLength := 10000; // needed e.g. from wrk/ab benchmarks
fHttpServer.ServerName := 'M'; // better plaintext performance
Copy link
Contributor

Choose a reason for hiding this comment

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

This optimization has been disallowed for other frameworks by the past:

We expect the Server header to be whatever is normal for the platform or framework. If the framework does not normally provide a Server response header, we nevertheless require that you provide one as this roughly normalizes network load across all implementations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Default Server: header produced by mORMot2 framework is mORMot2 (Linux) what is too long. So we override it to short one-letter version, as most other top-rated frameworks did, for example - .net use "K" instead of "Kestrel"

Copy link
Contributor

Choose a reason for hiding this comment

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

@pavelmash
Your server already has very very good performance: 6959479 req/sec.
Не занимайтесь ерундой.  

Copy link
Contributor

Choose a reason for hiding this comment

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

Must have been confused with the /p in plaintext routes. Thanks for confirming.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's disallowed to all play with the same rules.
But still not formalized in the rules and verification tests. :(

Copy link
Contributor

@joanhey joanhey Mar 22, 2023

Choose a reason for hiding this comment

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

Yes, I know.
It's for that, than we need to clarify it and add rule and verification.

Copy link
Contributor

Choose a reason for hiding this comment

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

For example, with Nginx you can't change the server name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Server header value is changed to mmt by [658165b] .
@joanhey - hope rule will be changed - I propose to use at last 3 character. For example google respond with server: gws, so 4 characters is too much.
And, for sure, we should add validation for new rule, to ensure all participants are in the same situation

Copy link
Contributor

Choose a reason for hiding this comment

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

We need to clarify this, today socketify.py uses Server: uWebSockets_20, we can remove Server header or add a minimal size like 3 characters, in my case I can change to Server: uWS

Copy link
Contributor

@remittor remittor Mar 24, 2023

Choose a reason for hiding this comment

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

@joanhey

For example, with Nginx you can't change the server name.

so you need to add additional characters to the response body to be honest

@@ -233,6 +234,7 @@ function TRawAsyncServer.GetRawRandomWorlds(cnt: PtrInt; out res: TWorlds): bool
pStmt.SendPipelinePrepared;
pConn.PipelineSync;
end;
pConn.Flush; // we use modified libpq what not flush inside PQPipelineSync - flush manually
Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds like a discussion that concluded by the requirement of SYNC calls. I would suggest you double check before using a modified libpq that prevents them.

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 checked. Requirements is to send a SYNC command, but not to flush client socket (do write syscall) after SYNC.
So we modify a lipbq to avoid socket flushing - see what we did.
At last Just-JS do the same (send all pipeline commands in one-two socket write) and it's implementation is valid

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sebastienros - checked once more using Postgres protocol verification tool as suggested in this post

Everything is green.

Copy link
Contributor

Choose a reason for hiding this comment

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

@pavelmash great then, thanks for taking the time to double-check and sorry for the inconvenience. I had forgotten about the CI verification step.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO will be forbidden to use a modified libpq.
We are testing frameworks !!

Copy link
Contributor

Choose a reason for hiding this comment

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

FYI my libpq patch is available here.

Copy link
Contributor

Choose a reason for hiding this comment

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

FYI

anyone should use any connectors, after all, optimizations are optimizations. The bad thing is to try to compare WebFrameworks with databases instead of direct testing the database connectors/solutions.

Copy link

@synopse synopse Mar 25, 2023

Choose a reason for hiding this comment

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

I don't think DB tests should be separated. Optimizations are indeed optimizations.
The real bad thing is that we compare frameworks which are proofs of concept, not usable in real-purpose code, with production ready frameworks.
For instance, some frameworks use a direct DB connection, but only handle integer fields, so that they could shine in TFB benchmarks. Or others don't allow to change the fortunes template without re-compiling the executable - which IMHO voids the aim of a HTML template, which is to separate view and model.
I would add a requirement with some complex process instead, something with data more complex than the /fortunes test, just to validate the framework is usable for any real usecase, i.e. is a true "realistic" framework.

Copy link
Contributor

@joanhey joanhey Mar 25, 2023

Choose a reason for hiding this comment

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

It's better to create a general issue or discussion, than talk in a PR.
We need to try to have a fair play, for all frameworks.
And later create individual issues, for each specific item.

@synopse not only the HTML template need compilation. I see some frameworks than cache the response in Fortunes, so bypassing the template, composition and html escape stage in each request.
We need optimizations, but general, not specific only to the benchmark.

PD: @pavelmash you can use only 1 letter server header, till is NOT added as a rule and verification.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@joanhey - reverted by [1a48475] to use 1 letter, thanks!
I agree what separate issue should be created for discussion.

Hope this PR will be merged now. At last this can help Postgres team to estimate changes we propose to libpq what implemented by @volyrique as libpq patch. In case Postgres team decline such patch I will rollback usage of modified libpq

@NateBrady23 NateBrady23 merged commit 9f43d30 into TechEmpower:master Mar 28, 2023
franz1981 pushed a commit to franz1981/FrameworkBenchmarks that referenced this pull request Jun 23, 2023
* [mORMot] using a modified libpq that allows to send the PipelineSync command without flushing prev. commands to socket

* [mORMot] fix dockerfile, speed up a docker build command

* [mORMot] shorten server name for better plaintext performance

* [mORMot] change `Server` header value to `mmt` because `M` provokes debate

* [mORMot] use 1 letter server header, till is NOT added as a rule and verification

---------

Co-authored-by: pavel.mash <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants