Skip to content

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:

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

coveralls commented Aug 26, 2023

Coverage Status

coverage: 92.795% (-0.07%) from 92.869%
when pulling aa56d4a on imap-token-cache
into 1d356d8 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

performance Improvements to speed or memory consumption

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants