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

Minimal API #15294

Merged
merged 7 commits into from
Mar 23, 2024
Merged

Minimal API #15294

merged 7 commits into from
Mar 23, 2024

Conversation

MikeAlhayek
Copy link
Member

@MikeAlhayek MikeAlhayek commented Feb 9, 2024

Fix #15274

@MikeAlhayek
Copy link
Member Author

MikeAlhayek commented Feb 10, 2024

@sebastienros I did bombarded by localhost using

bombardier -c 500 -n 1000000

Here is the output using mimimal API

image

Here is the output using controller

image

@sebastienros
Copy link
Member

5xx means all the responses are 500, so it's void. If you ran this command line it's missing the url.

bombardier -c 50 -d 15 -- insecure -l http://localhost:5000/{insertpathhere}

@MikeAlhayek
Copy link
Member Author

Oh see now that all of the are 5xx errr

bombardier -c 500 -n 1000000 https://localhost:44300/kb15/api/content/4efeywk0k48f1vbnewk0xwcpxd -H "Authorization: Bearer a token that was generated by postman"

let me try it again

@MikeAlhayek
Copy link
Member Author

@sebastienros here are better outputs using -c 125 -n 100000

Minimal APIs

image

Using controller

image

Copy link
Contributor

This pull request has merge conflicts. Please resolve those before requesting a review.

# Conflicts:
#	src/OrchardCore.Modules/OrchardCore.Contents/Controllers/ApiController.cs
@Piedone
Copy link
Member

Piedone commented Mar 21, 2024

This is not something ready for review, right? Because then please make it a draft.

@MikeAlhayek
Copy link
Member Author

@Piedone yes. I cleaned it up and fixed the test. It is ready for review. It uses Minimal API for the content API.

@Piedone
Copy link
Member

Piedone commented Mar 22, 2024

Is the primary goal of this performance?

@MikeAlhayek
Copy link
Member Author

Yes. And less complexity since we are using Minimal APIs.

@Piedone
Copy link
Member

Piedone commented Mar 22, 2024

I'm not sure about the complexity, since it's 25% more code than the controller was :).

Do we know though what makes it faster? Because if it's due to it not doing something that the MVC approach does, then we'll need to optimize that too, since we won't rewrite every OC API controller out there.

@MikeAlhayek
Copy link
Member Author

MikeAlhayek commented Mar 22, 2024

You are saving the complexity of creating the controller and its related objects.

Minimal API are executed directly like a middleware. We'll ensure that we only resolve services that are needed by the endpoint. If you see the Bombardier output I shared earlier, you'll see that it is able to handle slightly more requests that a controller does.

@Piedone
Copy link
Member

Piedone commented Mar 22, 2024

I see, thanks. Yep, seen that, it's nice.

@MikeAlhayek
Copy link
Member Author

I see, thanks. Yep, seen that, it's nice.

Did you want to review this PR?

@sebastienros can you please review this?

Copy link
Member

@Piedone Piedone left a comment

Choose a reason for hiding this comment

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

Do I understand correctly that there are no logic changes within the endpoints (compared to the actions)? Except for Delete returning 404 instead of 204 if no item is found.

@MikeAlhayek
Copy link
Member Author

Correct. At the end of the day, we took the code from a single controller and split it into multiple smaller endpoint level manageable files that get called quicker with less resources.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate to Minimal APIs
3 participants