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

Fix routerlicious timeouts in compression related tests #3

Conversation

pragya91
Copy link

@pragya91 pragya91 commented Jun 7, 2024

The timeouts for these tests were being caused by 2 things:

  • Large messages being sent
  • grouped batching being enable by default, thus breaking the logic in the tests that looks for empty messages

I reduced the size of messages being sent and their respective configs, as well as fixing the logic that tries to force a reconnect based on messages sent/received.

AB#7969
AB#7944

yann-achard-MS and others added 30 commits May 31, 2024 18:15
As a quick mitigation, we have a test that is causing our whole pipeline
to run into some serious locks.

This performance test has successfully caught a performance regression,
we have an item investigating and fixing that
[AB#8127](https://dev.azure.com/fluidframework/235294da-091d-4c29-84fc-cdfc3d90890b/_workitems/edit/8127)

I ran without this locally, and was able to get these tests to pass.
## Description

Add optional "source" to forest cursor creation. In ObjectForest, use it
to produce more descriptive errors about unexpected cursors.
These tests have been dead for quite a while, and their existence
creates more problems than potential value. We will come back and
reconsider options for visual regression testing in the near future.


[AB#8120](https://dev.azure.com/fluidframework/235294da-091d-4c29-84fc-cdfc3d90890b/_workitems/edit/8120)
)

add understanding of concurrently arguments that use wildcard * suffix
BREAKING CHANGE: A new tree status has been added for SharedTree nodes.

`TreeStatus.Created` indicates that a SharedTree node has been
constructed but not yet inserted into the tree.
Constraints passed to the `runTransaction` API are now marked as
`readonly`.
microsoft#20970)

Updated the following:

  client (release group)

Dependencies on @fluidframework/eslint-config-fluid updated:

  @fluidframework/eslint-config-fluid: 5.3.0

There were some code changes needed to adhere to the lint config. They
were partially automated then I manually fixed up others.
…1259)

Updates the policy config for the npm-package-json-script-dep policy so
it checks that `concurrently` is a dep when needed.

Also sorts the config alphabetically and adds typing to the config
object so we get IntelliSense and type-checking.
…alizeOrUpdateGCState (microsoft#21269)

Added peformance test to benchmark the peformance of
`initializeOrUpdateGCState` which is known to be expensive when number
of unreferenced nodes are high. This is due to the creation and
initialization of timer for every unreferenced node.

The test added benchmarks the peformance of `initializeOrUpdateGCState`
with 5000, 15000 and 30000 unreferenced nodes. Once the timers issue is
fixed, the performance of these tests should increase.
Policy:
- Package "exports" other than those under "./internal" should be linted
by api-extractor.
- A single "./internal" or alternatively "." export should also be
linted by api-extractor with a bundle config. Recently this was
"check:release-tags" and now is "check:exports:bundle-release-tags".

Unfortunately, api-extractor will only lint a single entrypoint at a
time and needs the config file to be local. So, this means lots of files
and script entries.

Build config:
 - Add "check:exports" to "lint" tasks.

Apply policy to client-utils and container-definitions.

[AB#8141](https://dev.azure.com/fluidframework/235294da-091d-4c29-84fc-cdfc3d90890b/_workitems/edit/8141)
…ach release level (except internal) (microsoft#21267)

Each package now generates API reports for each release level alpha,
beta, and public. These reports correspond with the trimmed roll-ups
API-Extractor generates for each release level. This offers convenient
insight into which APIs of a given package are available in which
entrypoint(s). It also offers a mechanism for tracking API items as they
are promoted to higher levels of support guarantees.

For now, we are not generating reports for internal-level exports. This
is intentional but does not have to be a permanent change. If we decide
in the future that it would be valuable to track our internal API
surfaces, we can add that level to the set of reports we generate.

These changes also exclude the packages under `build-tools`, as those
packages don't leverage type-trimming, nor do they manage multiple
entrypoints.


[AB#7523](https://dev.azure.com/fluidframework/235294da-091d-4c29-84fc-cdfc3d90890b/_workitems/edit/7523)

### Some notes for reviewers

1. Reports for empty package APIs are still generated, and simply don't
contain export details. We may wish to file a feature request to make
this a bit cleaner. Probably append a notice that there are no exports
at the given release level.
2. "Trimmed" reports will contain "untrimmed" imports. This matches the
behavior of the corresponding typings rollups, but it's not particularly
interesting from a report perspective, and could create noise in
automated review enforcement. We should probably file a feature request
around this too, possibly just a way to opt out of including imports in
the report file (I don't think we generally want them there anyways).
This small change will make it easier for future implementations of
"flex trees" to access `indexForAt`.
## Description

Unify `IDisposable` interfaces from `@fluidframework/tree` and
`@fluidframework/core-interfaces`.

## Breaking Changes

Public APIs in `@fluidframework/tree` now use `IDisposable` from
`@fluidframework/core-interfaces` replacing `disposeSymbol` with
"dispose".

`IDisposable` in `@fluidframework/core-interfaces` is now `@sealed`
indicating that third parties should not implement it to reserve the
ability for Fluid Framework to extend it to include `Symbol.dispose` as
a future non-breaking change.
BREAKING CHANGE:

`TreeStatus.Created` has been renamed to `TreeStatus.New`.
…es and snapshots (microsoft#21264)

Added a document (.md file) to the container runtime's summary folder.
it describes the format of summaries and snapshots. This should help
developers understand how to look at the data, how to make changes, etc.
It should also help users understand the format of their data in summary
/ snapshot.

The documentation goal is to have a README.md in the same folder that
describes overall summarization and it links to this document.
## Description

Make experimental packages consistent with how they extends the base
tsconfigs.
## Description

Our default target is es2020. There is no need to explicitly target
older than that.

## Breaking Changes

Devtool and build-common may now require ES 2020, just like all other
client packages.
## Description

This adds a new implementation of `FlexTreeNode` (and `FlexTreeField`,
etc.) which wraps a tree of `MapTrees` and allows their data to be read
via the flex APIs. This will allow us to support the reading of
unhydrated SharedTree nodes in the future.
Bumping server dependencies to prerelease version in the client release
group to access new [targeted signal
capabilities](microsoft#19519) on
the client side.

Will need confirmation from FF release engineer(s) since they will be
the person who will have to do the real server release.

I used the following command to complete the bump from the repo root:  
`flub bump deps server -g client -t greatest --prerelease`

I then did `pnpm install` to update the pnpm-lock file.
Change doc references for `AzureUser` as `AzureUser` is internal which
prevents `@inheritDoc AzureUser...` resolution.


[AB#8135](https://dev.azure.com/fluidframework/internal/_workitems/edit/8135)
[AB#8163](https://dev.azure.com/fluidframework/235294da-091d-4c29-84fc-cdfc3d90890b/_workitems/edit/8163)

Reduce e2e pipeline duration for FRS and routerlicious testing (although
it may not have an impact as odsp lock tests still will take the
longest).
Currently if a local interval from `clients[0]` is rebased over a
removal from `clients[1]` which removes the entire contents of the
interval, and that interval ends up at one of the extents of the
sequence (the beginning or end), that local interval is removed and
never sent to other clients. If this causes the interval collection to
become empty this creates an inconsistency where only `clients[0]` would
see the collection label in `getIntervalCollectionLabels`.

If `mergeTreeReferencesCanSlideToEndpoint` is enabled, we should treat
intervals at start/end just like any other empty interval and still send
it to all the other clients.
microsoft#21135)

## Description

We currently store the entire pending message in `savedOps` despite not
needing its local op metadata at usage. We noticed this memory leak in
some SharedTree usage, which puts a copy of the tree's schema in local
op metadata (schema has cheap copy, but still seemed reasonable to fix).

Co-authored-by: Abram Sanderson <[email protected]>
…nce pipeline (microsoft#21302)

I had added some GC performance benchmark tests via
https://github.com/microsoft/FluidFramework/pull/21269/files.

In order to run these tests in the performance pipeline, couple more
steps are needed. This PR adds these steps:
- Added a "postpack" script to the container-runtime package which puts
the package's test files (src, lib, and dist) in a tar file, which the
perf benchmarks pipeline then consumes
- Listed the container-runtime package in the yaml file that should run
in the pipeline.
…soft#20852)

## Description

Adds the recommendation that users set initial state before attaching
any created DDSes to `IFluidContainer.create`, and include a code
example.

---------

Co-authored-by: Abram Sanderson <[email protected]>
Co-authored-by: alex-pardes <[email protected]>
DLehenbauer and others added 8 commits June 7, 2024 07:36
…icrosoft#21318)

Removes client dependence on 'protocol-base' for utility functions that
work with Git summary trees.

No suprises / very straightforward.
1. Stop exporting a "defaultWebpackConfig", instead config the examples
independently.
2. Update source-map-loader dependency to 5.0.0 throughout the repo
(including one usage in server for a test while I'm at it). Remove it as
an unused dependency for several packages.
3. Remove bohemiaIntercept, unused.
4. Remove isSynchronized.  isSynchronized had a sneaky
use through the window object. The test now uses a timeoutPromise
strategy instead.
5. Remove webCodeLoader and replace with StaticCodeLoader.
6. Fix stale comments in modelLoader.ts.
The docs pipeline fails because the scope of odsp-client is changed from
experimental to fluidframework. This change should fix the pipeline.
…t#21342)

## Description

This PR changes unhydrated nodes to be backed by MapTreeNodes to allow
them to be read even before insertion.
…icrosoft#21282)

#### Description

[ADO
8152](https://dev.azure.com/fluidframework/internal/_workitems/edit/8152/)
[ADO
5141](https://dev.azure.com/fluidframework/internal/_workitems/edit/5141/)

This PR creates telemetry loggers for the `SharedTree` DDS which emit
telemetry events for rebase operations of below scenarios:
- rebasing the EditManager local branch over new peer edits
- rebasing an EditManager peer branch over trunk edits when adjusting
the branch to the peer's new ref seq
- rebasing a new peer edit to the tip of the trunk
- rebasing the inverse of a revertible.
- rebasing a user branch

The event will include:
- parent branch length
- child branch length
- time duration

**Note**: Instead of logging the event for __every__ rebase operation,
we should log events in batch (e.g., every 100 operations).

---------

Co-authored-by: yann-achard-MS <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment