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.

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

Added Benchmarks for all engines #238

Merged
merged 6 commits into from
May 1, 2023
Merged

Added Benchmarks for all engines #238

merged 6 commits into from
May 1, 2023

Conversation

ytsruh
Copy link
Contributor

@ytsruh ytsruh commented Apr 18, 2023

Added benchmakrs to all engines (except Mustache extended). A summary & charts were added to the README file for future reference

This is a full PR that should hopefully satisfy #227

ytsruh added 3 commits April 14, 2023 21:46

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@ytsruh ytsruh marked this pull request as ready for review April 18, 2023 18:26
@efectn
Copy link
Member

efectn commented Apr 19, 2023

Can you add benchmark for slim engine

@ytsruh
Copy link
Contributor Author

ytsruh commented Apr 19, 2023

Happy to add them, however all of slim_test.go file was commented out & the Slim engine wasn't mentioned in the docs. Do you know which implementation of slim is used and I can add them tonight?

@efectn
Copy link
Member

efectn commented Apr 19, 2023

Happy to add them, however all of slim_test.go file was commented out & the Slim engine wasn't mentioned in the docs. Do you know which implementation of slim is used and I can add them tonight?

Yeah it doesn't have docs and i've never used it before. Maybe we should try to uncomment tests and add benchmark. We will add README later

@ytsruh
Copy link
Contributor Author

ytsruh commented Apr 19, 2023

I'll give it a try and see how I get on (no promises though!)

@ytsruh
Copy link
Contributor Author

ytsruh commented Apr 19, 2023

OK so I had look over this and I think I know why it was commented out, I dont think the implementation works as is expected - it seems to add additional div elements to layouts. See below:
Screenshot 2023-04-19 at 17 54 59
On top of this the control flow from the Go implementation appears to not work, at least not using the syntax of the original engine. The two references I used are:

With all of this in mind, I'm not sure the currently able to add the Benchmark. If I did it wouldn't be particularly useful.

Would be ok if we split Slim engine benchmark to its own issue? And somebody can work on the implementation of the engine and then we can add the benchmark afterwards?

@efectn
Copy link
Member

efectn commented Apr 19, 2023

OK so I had look over this and I think I know why it was commented out, I dont think the implementation works as is expected - it seems to add additional div elements to layouts. See below: Screenshot 2023-04-19 at 17 54 59 On top of this the control flow from the Go implementation appears to not work, at least not using the syntax of the original engine. The two references I used are:

With all of this in mind, I'm not sure the currently able to add the Benchmark. If I did it wouldn't be particularly useful.

Would be ok if we split Slim engine benchmark to its own issue? And somebody can work on the implementation of the engine and then we can add the benchmark afterwards?

Ok, let's skip it for now

@ReneWerner87
Copy link
Member

@ytsruh can you pls check the tests

Copy link
Member

@efectn efectn left a comment

Choose a reason for hiding this comment

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

Don't forget to delete ds store

@ytsruh
Copy link
Contributor Author

ytsruh commented Apr 22, 2023

Hi guys, sorry about the delay in fixing the tests. All passing now.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@ReneWerner87
Copy link
Member

will check it on sunday at the latest

@ReneWerner87 ReneWerner87 merged commit 7a885a8 into gofiber:master May 1, 2023
@ReneWerner87 ReneWerner87 linked an issue May 1, 2023 that may be closed by this pull request
@ReneWerner87
Copy link
Member

maybe we should use
https://github.com/marketplace/actions/continuous-benchmarking-for-go
for the benchmark visualisation

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.

Benchmarks for all template engines
3 participants