microsoft / FluidFramework

Library for building distributed, real-time collaborative web applications
https://fluidframework.com
MIT License
4.7k stars 528 forks source link

[GC] Removed unreferenced event Q for summarizer and log unreferenced events only if node was not revived first #21961

Closed agarwal-navin closed 1 month ago

agarwal-navin commented 1 month ago

Reviewer guidance

This is targetting a test branch (test/gc-2.3). Since this has legacy alpha API breaking changes which are not allowed to be merged until v2.3, this is a workaround to get the changes reviewed and merged in a test branch. Later when breaking changes are allowed, these will get merged to main.

Description

In summarizer clients, unreferenced events (inactive, tombstone ready and sweep ready) are added to a queue and are logged after the next GC completes. This was done to reduce false positives in 2 scenarios:

With the reference op work, both of these problems don't exist. So, we can remove the unreferenced event queue for summarizer clients. Also, we can check that an object is not revived before we log an event for it which will reduce false positives for non-summarizer clients too.

This PR makes 3 main changes:

  1. It removes the unreferenced event queue for summarizers and logs unreferenced events right away.
  2. It checks that GC did not see a reference for an unreferenced object before it was used. If it did, it does not log because the usage isn't unexpected.
  3. Added packagePath and fromPackagePath properties to addedGCOutboundRoute. Since this information is already present with the runtime when it calls GC, it doesn't need to ask for it.

AB#8938

azure-boards[bot] commented 1 month ago

✅ Successfully linked to Azure Boards work item(s):

azure-boards[bot] commented 1 month ago

✅ Successfully linked to Azure Boards work item(s):

agarwal-navin commented 1 month ago

@microsoft/fluid-cr-api Note that this is against a test branch and not main.

agarwal-navin commented 1 month ago

As discussed with @markfields offline and per this comment https://github.com/microsoft/FluidFramework/pull/21961#discussion_r1689055623, closing this PR for now. This can regress behaviors and can be more costly than it's worth. We can revisit this later and remove the whole thing once GC is stable. We will only need tombstone and deleted logs then.