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

Query: Port Include() performance improvement to 1.0.2 #6760

Closed
divega opened this issue Oct 12, 2016 · 13 comments
Closed

Query: Port Include() performance improvement to 1.0.2 #6760

divega opened this issue Oct 12, 2016 · 13 comments
Assignees
Labels
area-perf closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-enhancement
Milestone

Comments

@divega
Copy link
Contributor

divega commented Oct 12, 2016

We made a fix in d013493 that significantly improves the performance of certain queries with Include().

We should check if it meets the risk bar for a patch release.

@gdoron
Copy link

gdoron commented Oct 14, 2016

In general, I think the releases should be more often and release as soon as possible.
I'm not sure why, but sometimes, there are very small changes, that surely won't break anything with great impact, but they are merged to a branch that is going to be release in 4 months!

An example (from the MVC repo) @dougbu: aspnet/Mvc@cbc88fc
aspnet/Mvc#4989

Please bear in mind there are real applications out there, which crucially need those fixed merged, and 4 months or even more to wait is sometimes unacceptable, depending on the severity.

Thanks for considering it @divega !

@rowanmiller
Copy link
Contributor

@anpete can you evaluate the risk on this one

@anpete
Copy link
Contributor

anpete commented Oct 14, 2016

@rowanmiller Low risk. I think we can take it.

@rowanmiller
Copy link
Contributor

Cool, can you port the fix into 1.0.2

@anpete
Copy link
Contributor

anpete commented Oct 17, 2016

Merged

@anpete anpete closed this as completed Oct 17, 2016
@anpete anpete added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Oct 17, 2016
@rowanmiller rowanmiller changed the title Consider porting Include() perf improvement to 1.0.2 Port Include() performance improvement to 1.0.2 Oct 17, 2016
@gdoron
Copy link

gdoron commented Oct 17, 2016

Thanks 👍 👍 👍

@divega
Copy link
Contributor Author

divega commented Oct 17, 2016

Reopening to take through patch release approval. @anpete could you please write a risk assessment for the change? I will send you samples 😄

cc @Eilon

@Eilon
Copy link
Member

Eilon commented Nov 1, 2016

This patch is approved, please ensure it is merged into the correct branch and building as part of the patch train.

@divega
Copy link
Contributor Author

divega commented Nov 2, 2016

This is already in all the right branches.

@divega divega closed this as completed Nov 2, 2016
@gdoron
Copy link

gdoron commented Nov 2, 2016

@Eilon @divega Can you share a ~ release date of 1.0.2?
We just got hit hard by the bug I mentioned earlier of MVC and ValidationSummary. 😢

(Safari for some reason doesn't respect form validations (for 9 years!) and a very important customer of ours submitted an invalid form and MVC couldn't show him the errors because the required field was in a collection)

@Eilon
Copy link
Member

Eilon commented Nov 2, 2016

@gdoron hopefully soon 😄 We're nearly done writing the code, but we have to do verification of the bits first. We have a package feed with nightly builds at https://dotnet.myget.org/gallery/aspnetcore-patch, but the builds for this release aren't there yet (we're working on it right now).

@gdoron
Copy link

gdoron commented Nov 2, 2016

@Eilon, for some reason, we can't use the nightly builds.
When we try to update our references it always breaks and won't compile, it's extremely frustrating 😢 .
@smitpatel was very kind and tried helping us out, but it didn't work for us.

Can you please assign someone to take a look? I can give him an access to our private GitHub repository (Yes, we are desperate...) so it won't be a trial and error.

p.s. It's for a very good cause, ASP.NET Core is not just saving kittens 😸 but helping people with disabilities at yooocan.com 💪.
Checkout this amazing story we got two days ago for Halloween: https://yooocan.com/Story/329/Having-Wheels-Has-Its-Perks.

@Eilon
Copy link
Member

Eilon commented Nov 7, 2016

@gdoron well if you do manage to get nightly builds working, we have some builds on this feed: https://dotnet.myget.org/gallery/aspnetcore-patch

As far as dates, we're nearly done with the code, but the actual release involves a number of teams coordinating lots of builds so we can't guarantee any particular dates.

BTW very cool project you're working on!

@divega divega changed the title Port Include() performance improvement to 1.0.2 Query: Port Include() performance improvement to 1.0.2 Dec 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-perf closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-enhancement
Projects
None yet
Development

No branches or pull requests

6 participants