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

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

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

jstedfast
Copy link
Owner

@jstedfast jstedfast commented Aug 26, 2023

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

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

TODO:

  • Find a way to get rid of ImapToken's internal IsAscii() method - it is likely killing perf. We either need to be able to handle non-ascii tokens (which I think can only be qstring tokens) or else we need to track ascii-vs-non-ascii while adding bytes to the ByteArrayBuilder so that we can skip re-scanning the token data.
  • 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?

@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
Copy link

coveralls commented Aug 26, 2023

Coverage Status

coverage: 93.969% (-0.1%) from 94.066%
when pulling 7dcbda2 on imap-token-cache
into 2a42387 on master.

@jstedfast
Copy link
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
Also reduced memory allocations in ImapTokenCache.AddOrGet()
…lders

This drastically reduces the number of allocations made when tokenizing
IMAP responses.

ImapCommand's usage was not really a major issue, but since ImapEngine.ReadLine/Async()
needed a reusable ByteArrayBuilder anyway, might as well share that with ImapCommand.
…own too large

One potential problem with reusing ByteArrayBuilders is that, because they can
grow for some abnormally long tokens/lines/commands/etc, those oversized buffers
will remain referenced by the ImapStream/ImapEngine until they are disposed which
could be the life of the program.

If we oportunistically scale back the size of the buffers when they are Clear()'d,
then we can periodically reduce memory usage and allow those larger buffers to be
used elsewhere.
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.

2 participants