- 
                Notifications
    
You must be signed in to change notification settings  - Fork 432
 
Reduce memory consumption by avoiding adding items when queue is full. #3239
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
base: dev
Are you sure you want to change the base?
Conversation
| /// </summary> | ||
| /// <returns>the time when the event queue task should end</returns> | ||
| private DateTime SetTaskEndTime() | ||
| public bool SetValue(TKey key, TValue value) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider naming it TrySetValue to indicate the operation could succeed/fail
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sruke Good idea.
I kept the older methods as we had shipped them.
a3abd7e    to
    affe253      
    Compare
  
    Do not add items if cache is full or compacting.
3f298b4    to
    f7aeb05      
    Compare
  
    
          SummarySummary
 CoverageMicrosoft.IdentityModel.JsonWebTokens - 80.3%
  | 
    
| private int _removeExpiredValuesState = ActionNotQueued; | ||
| private int _processCompactedValuesState = ActionNotQueued; | ||
| private readonly Task _eventQTask; | ||
| private int _eventQueuePollingInterval = 50; // in milliseconds | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| private int _eventQueuePollingInterval = 50; // in milliseconds | |
| private readonly int _eventQueuePollingInterval = 50; // in milliseconds | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks right, there are a few failing tests that need fixed, otherwise LGTM
| 
           @brentschmaltz please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information. 
 Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”), 
 
 
  | 
    
Do not add items if cache is full or compacting.
Under heavy load, stress tests showed that the LRUCache was unable to keep up a change was made to avoid adding to the cache if we detect it was full or compacting.
A change to start TaskEventQ on startup instead of starting and stopping improves performance and simplifies the code.
We were not checking is a SignatureProvider was actually added to the cache. This is important as if the SignatureProvider is not added, the cache will not be able to properly apply the dispose logic and kernel objects could pile up.