Skip to content

Added an ImapToken cache to try and reduce ImapToken allocations - #1630

Open
jstedfast wants to merge 1 commit into
masterfrom
imap-token-cache
Open

jstedfast wants to merge 1 commit into
masterfrom
imap-token-cache

Conversation

@jstedfast

@jstedfast jstedfast commented Aug 26, 2023

Copy link
Copy Markdown
Owner

Theoretically, this could drastically reduce ImapToken allocations which might help with issue #1335.

Of course, this also adds more overhead in other aspects.

TODO:

  • Write unit tests for ImapTokenCache
  • Play with different cache limits to see if there is an optimal limit
  • Move the cache to be ImapEngine-specific? Might be better than a global ImapToken cache like it is now. Another advantage of that would be that the mutex locking wouldn't be necessary anymore. Not sure how expensive locking is...
  • Benchmark?

Alternative Idea:

Instead of (or in combination with?) the ImapTokenCache, if we used ReadOnlyMemory<byte>, we wouldn't need:

  1. to allocate a token buffer
  2. to copy the token into the token buffer

This would reduce memory allocations, memory fragmentation, and improve performance because there'd be no (or less) need to copy data around.

That said, there are a few things to consider:

  1. QString tokens would still need to be copied (because we unquote them as we copy them) or we would need to change the code that handles qstring tokens to unquote as they are consumed.
  2. long tokens that require re-filling the buffer would add complexity and, in worst-case scenarios, might end up with tokens longer than the I/O input buffer, requiring us to allocate a "token buffer" anyway. It should be exceptionally rare since the I/O buffer is 4K, but we still need to handle this scenario somehow - even if it is to just throw an exception (which may be a perfectly legitimate thing to do).
  3. The current IMAP implementation has situations where we can unget a series of tokens (I think up to 3?) which would be risky since it's possible a second or third token would require refilling the buffer which would "corrupt" the older token(s). We need to handle this somehow.

@jstedfast
jstedfast temporarily deployed to ci August 26, 2023 20:46 — with GitHub Actions Inactive
@jstedfast
jstedfast temporarily deployed to ci August 26, 2023 20:46 — with GitHub Actions Inactive
@jstedfast
jstedfast temporarily deployed to ci August 26, 2023 20:46 — with GitHub Actions Inactive
@coveralls

coveralls commented Aug 26, 2023

Copy link
Copy Markdown

Coverage Status

Coverage is 92.587%imap-token-cache into master. No base build found for master.

@jstedfast

Copy link
Copy Markdown
Owner Author

How much, in terms of allocations, does the ImapTokenCache save us?

No cache (for comparison):
NoImapTokenCache

With this PR (cache capacity = 128):
WithImapTokenCache

capacity = 256:
WithImapTokenCache256

@jstedfast
jstedfast temporarily deployed to ci August 27, 2023 02:54 — with GitHub Actions Inactive
@jstedfast
jstedfast temporarily deployed to ci August 27, 2023 02:54 — with GitHub Actions Inactive
@jstedfast
jstedfast temporarily deployed to ci August 27, 2023 02:54 — with GitHub Actions Inactive
Comment thread MailKit/Net/Imap/ImapTokenCache.cs Fixed
Comment thread MailKit/Net/Imap/ImapTokenCache.cs Fixed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

performance Improvements to speed or memory consumption

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants