Skip to content
This repository has been archived by the owner on Nov 20, 2018. It is now read-only.

Allow feature cache to be updated #501

Closed
wants to merge 1 commit into from
Closed

Allow feature cache to be updated #501

wants to merge 1 commit into from

Conversation

benaadams
Copy link
Contributor

No point in pooling if you can't actually update stuff; allows for a saving of 959KB per 4k requests

See: https://github.com/aspnet/benchmarks/pull/34/files#diff-a48f9d66e0bc9b9c9ec15dae230d8e50R167
in aspnet/Benchmarks#34 for specific usage

Resolves #502
Resolves #402

Allocation details when pooling (in benchmarks repo), usual stuff, 4k requests, 16 pipelined, using FF

Pre-allocations for DefaultContext 1.12MB

before-allocs

Post-allocations for DefaultContext 160KB

after-allocs

Almost all of which is logging scope

before-allocs

benaadams added a commit to benaadams/benchmarks that referenced this pull request Dec 6, 2015
@benaadams
Use PooledContextFactory
1b081b7 
Also needs aspnet/HttpAbstractions#501 to be
real
@benaadams benaadams mentioned this pull request Dec 6, 2015
Closed
benaadams added a commit to benaadams/benchmarks that referenced this pull request Dec 7, 2015
@benaadams
Use PooledContextFactory
2068ce6 
Also needs aspnet/HttpAbstractions#501 to be
real
benaadams added a commit to benaadams/benchmarks that referenced this pull request Dec 7, 2015
@benaadams
Use PooledContextFactory
8c7e97f 
Also needs aspnet/HttpAbstractions#501 to be
real
davidfowl
davidfowl reviewed Dec 7, 2015
View reviewed changes
src/Microsoft.AspNet.Http/Features/IFeatureCache.cs
{
void CheckFeaturesRevision();
void ReplaceFeatures(IFeatureCollection features);
Copy link
Member

Choose a reason for hiding this comment

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

UpdateFeatures

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

benaadams added a commit to benaadams/benchmarks that referenced this pull request Dec 7, 2015
@benaadams
Use PooledContextFactory
9fceb96 
Also needs aspnet/HttpAbstractions#501 to be
real
Tratcher
Tratcher reviewed Dec 7, 2015
View reviewed changes
src/Microsoft.AspNet.Http/DefaultHttpContext.cs

_authenticationManager?.UpdateFeatures(features);
_connection?.UpdateFeatures(features);
_websockets?.UpdateFeatures(features);
Copy link
Member

Choose a reason for hiding this comment

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

Should these lazy sub-objects be cleared out per request? Only some requests will need them. If they don't get cleared then eventually every pooled HttpContext will have all of 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.

Paid the price; fairly small, are a defined size (rather than an unknown user code type) - might as well? (Happy either way)

Copy link
Member

Choose a reason for hiding this comment

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

shrug It's a probability question. We can figure it out based on profiles later.

@benaadams benaadams mentioned this pull request Dec 7, 2015
Closed
@benaadams benaadams changed the title Allow feature cache to be updated+invalidated Allow feature cache to be updated Dec 8, 2015
davidfowl
davidfowl reviewed Dec 8, 2015
View reviewed changes
src/Microsoft.AspNet.Http/Features/IFeatureManager.cs

namespace Microsoft.AspNet.Http.Features
{
public interface IFeatureManager
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this new public interface?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, but would be implementing without contract; which is probably fine :)
Removed.

@davidfowl
Copy link
Member

@benaadams We need some tests for this to make sure all of our features properly update and nothing is cached (unexpectedly).

@benaadams
Copy link
Contributor Author

... and nothing is cached (unexpectedly).

Thinking future proofing... reflection to discover interfaces and make sure they are null?

Tratcher
Tratcher reviewed Dec 8, 2015
View reviewed changes
src/Microsoft.AspNet.Http/Features/QueryFeature.cs
@@ -9,7 +9,7 @@ namespace Microsoft.AspNet.Http.Features.Internal
{
public class QueryFeature : IQueryFeature, IFeatureCache
{
private readonly IFeatureCollection _features;
private IFeatureCollection _features;
Copy link
Member

Choose a reason for hiding this comment

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

The changes to these Feature classes can be reverted, especially _features. ResetFeatures doesn't matter much one way or the other.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@Tratcher
Copy link
Member

Tratcher commented Dec 8, 2015

Looking good, but yes it still needs some tests.

benaadams added a commit to benaadams/benchmarks that referenced this pull request Dec 10, 2015
@benaadams
Use PooledContextFactory
b9728d4 
Also needs aspnet/HttpAbstractions#501 to be
real
@benaadams
Copy link
Contributor Author

Added test; with it, found some areas where it wasn't actually caching and some where it was over clearing, so fixed those too.

davidfowl
davidfowl reviewed Dec 11, 2015
View reviewed changes
test/Microsoft.AspNet.Http.Tests/DefaultHttpContextTests.cs
@@ -147,6 +150,140 @@ public void SetItems_NewCollectionUsed()
Assert.Same(item, context.Items["foo"]);
}

#if DNX451
Copy link
Member

Choose a reason for hiding this comment

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

Why is this ifdefed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

coreclr doesn't have IsInterface in reflection

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh, are some extension methods on .GetTypeInfo()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Enabled for all frameworks now

davidfowl
davidfowl reviewed Dec 11, 2015
View reviewed changes
src/Microsoft.AspNet.Http/Authentication/DefaultAuthenticationManager.cs
}
}

void IFeatureCache.SetFeaturesRevision()
Copy link
Member

Choose a reason for hiding this comment

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

Why the explicit implementation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't want someone to call it; as it says "mark cache as up to date; but don't check it" - its only valid use is as part of the Interface contract.

@benaadams
Copy link
Contributor Author

Changes made

@Tratcher Tratcher self-assigned this Dec 16, 2015
@Tratcher Tratcher added this to the 1.0.0-rc2 milestone Dec 16, 2015
@Tratcher
Copy link
Member

Rebased and merged.

@Tratcher Tratcher closed this Dec 16, 2015
@benaadams
Copy link
Contributor Author

w00p! 👍

@benaadams benaadams deleted the feature-cache-update branch December 16, 2015 01:17
benaadams added a commit to benaadams/benchmarks that referenced this pull request Jan 27, 2016
@benaadams
Use PooledContextFactory
82395c6 
Also needs aspnet/HttpAbstractions#501 to be
real
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees

@Tratcher Tratcher

Labels
None yet
Projects
None yet
Milestone
1.0.0-rc2
Development

Successfully merging this pull request may close these issues.
4 participants
@benaadams @davidfowl @Tratcher @dnfclas