Skip to content

ForwardMsgCache expiration#82

Merged
tconkling merged 20 commits intostreamlit:feature/hashingfrom
tconkling:tim/MessageCacheEviction
Sep 17, 2019
Merged

ForwardMsgCache expiration#82
tconkling merged 20 commits intostreamlit:feature/hashingfrom
tconkling:tim/MessageCacheEviction

Conversation

@tconkling
Copy link
Copy Markdown
Contributor

This implements ForwardMsgCache expiration on both the client and the server.

  • The global.maxCachedMessageAge config value specifies the maximum age of a message in the cache. The age of a message is defined by the number of times a referencing report has finished running since the message was accessed by that report. So when a report finishes running for the first time, the age of all its cached messages is 1.
  • Our default maxCachedMessageAge is 2, which means that messages will remain cached even after not having been referenced for a report run.
  • To determine the age of messages in its cache, the server tracks a report_run_count alongside each ReportSession in its websocket->reportsession dictionary. The server increments this value each time it processes a report_finished ForwardMsg message for a report.
  • When the server increments report_finished, it also asks the cache to remove any expired messages. I haven't done anything fancy here - the cache just iterates through all its entries, does an age check, and deletes them if they're expired. @tvst, I know you were potentially concerned about performance issues with the "iterate the whole cache" strategy. We could add a simple timer to this function to collect local metrics to see if it's actually an issue?
  • Similarly, when the client receives a report_finished ForwardMsg, it performs the same cache expiration step. This means that the server and client's caches should stay in sync. (I haven't thought hard enough about whether it's possible for them to get briefly out of sync, but if they do, we have the new /message endpoint as a fallback.)
  • If a report fails to run due to a compilation error, neither the server nor the client will increment the report_run_count for that session. (The run-count incrementing is conditional on the status value of the report_finished message, which is set to FINISHED_WITH_COMPILE_ERROR when that happens.)

There are client and server tests for all this new logic. But the easiest way to see the expiration in action is to run something like examples/core/message_deduping.py and watch the debug output on the server and the client. (You'll see logs for cache hits and misses - you should never see a miss! - on the client; and logs for sending cached message refs on the server.)

  • This example just creates a big dataframe and sends it twice
  • The first time the report is run, the message will only be delivered to the client once.
  • If you rerun the report, the message won't be delivered at all because it'll be cached.
  • If you remove the st.dataframe() calls and rerun the report, the dataframe will not be removed from the cache because it won't be old enough. Re-adding those calls and rerunning the report should confirm this.
  • If you remove the dataframe calls and re-run the report twice with them missing, they will then be expired from the cache. Re-adding them should result in the message being resent to the client.

@tconkling tconkling requested review from monchier and tvst September 10, 2019 18:53
Copy link
Copy Markdown
Contributor

@tvst tvst left a comment

Choose a reason for hiding this comment

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

Still reviewing this, but wanted to send out some comments.

Copy link
Copy Markdown
Contributor

@tvst tvst left a comment

Choose a reason for hiding this comment

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

LGTM after fixes

@tconkling tconkling merged commit bbbb286 into streamlit:feature/hashing Sep 17, 2019
@tconkling tconkling deleted the tim/MessageCacheEviction branch September 17, 2019 18:10
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