Prefetch snapshot so as to avoid network call from critical path. #5968
Conversation
|
Still need to add tests. Just creating a PR so that people can take a look and give suggestions. |
|
Do we have an easy way to see the bundle size of just importing the prefetch API on the host side? I know FF has some issues with tree shaking in a few places |
The bundle size change will get mentioned in this PR once the build completes. |
packages/drivers/odsp-driver/src/odspDocumentServiceFactoryCore.ts
Outdated
Show resolved
Hide resolved
It will get added for existing code paths, but I'm curious the bundle size of just consuming the new API. I would recommend reaching out to @heliocliu to see the best way to hook up this new API to bundle analysis |
|
Can we incorporate this new flow into packages\test\test-service-load somehow? Or maybe (in addition to it?) add to one of the examples / web loader? I.e. something that we can use to manually hit this path, as we have no test coverage at all and it's just a matter of time till we break it (or do not find that it does not work as is) |
| import { PerformanceEvent } from "@fluidframework/telemetry-utils"; | ||
| import { IOdspSnapshot } from "./contracts"; | ||
|
|
||
| export function evalBlobsAndTrees(snapshot: IOdspSnapshot) { |
There was a problem hiding this comment.
Rather than an odspUtils2.ts, what about just creating two separate files: evalBlobsAndTrees.ts and toInstrementedOdspTokenFetcher.ts?
There was a problem hiding this comment.
Actually I added these to odsputils only because we already import a lot from that file. So no need to add these in a separate file.
|
@christiango I added the api in bundle size analysis after discussing with @heliocliu. The increase is just 2.12 KB. |
@heliocliu total size looks wrong, its looks be counting bytes as KB. Also, @christiango we stamp bundle size changes on the PR, but need successfull build |
The total size seems correct to me, it's presumably the sum of all other entrypoints + any dynamic imports. Yeah, I'm guessing we just don't represent newly added bundles since we don't have a basline to compare against. But given the total size change it seems like this preload API will be a ~30KB hit to the bundle. @Zlatkovsky - might be worth reaching out to teams folks to see if this 30KB hit would be problematic for the functionality we want to add. |
| @@ -127,6 +128,9 @@ export interface OdspFluidDataStoreLocator extends IOdspUrlParts { | |||
| fileVersion?: string; | |||
| } | |||
|
|
|||
| // @public | |||
| export function prefetchLatestSnapshot(resolvedUrl: IResolvedUrl, getStorageToken: TokenFetcher<OdspResourceTokenFetchOptions>, persistedCache: IPersistedCache, logger: ITelemetryBaseLogger, hostSnapshotFetchOptions: ISnapshotOptions | undefined): Promise<boolean>; | |||
There was a problem hiding this comment.
@vladsud is this the right package for this? should this be dynamically loaded? are there versioning implication for this
There was a problem hiding this comment.
It's fine for now. Basically only driver factory and driver factory input/output times (that are all defined in odsp-driver-definitions) are subject for dynamic loading.
Long term yes, we should separate driver (implementation) into multiple packages, and likely move out error handling code out of odsp-doclib-utils package into its own package.
The tests also validate only that factory is compatible with old versions of loader / runtime (merged this morning).
■ @fluidframework/base-host: No change
⯅ @fluid-example/bundle-size-tests: +25.39 KB
Baseline commit: 3f262e4 |
|
|
||
| export function apisToBundle() { | ||
| // eslint-disable-next-line @typescript-eslint/no-floating-promises | ||
| prefetchLatestSnapshot(undefined as any, undefined as any, undefined as any, undefined as any, undefined); |
There was a problem hiding this comment.
Emails suggested that this produced 30kb bundle. Is this really the case? There is not much code in here.
It's worth taking a look on why it's so large. I assume numbers are for non-minimized code, right?
There was a problem hiding this comment.
It should be minified code. I would recommend having someone take a look of the output of a plugin like this when we run just this api through webpack in production mode: https://github.com/webpack-contrib/webpack-bundle-analyzer
There was a problem hiding this comment.
This prefetchSnapshot api file seems to be about ~10Kb and then almost all utilities are used from odspUtils which also seems to account for ~10KB and then there are some other imports from driver-defn, assert, uuid etc which could have accounted for 10Kb more.
There was a problem hiding this comment.
@heliocliu These are non-minimized bundle size, right?
There was a problem hiding this comment.
The metric is for minified code. The 30kb includes all its dependencies (and their dependencies, etc) as well. If a host were to take a dependency on this method without already having any dependencies on this method's dependencies, the host would incur a 30kb size increase. Like mentioned before, the tool doesn't seem to handle new metrics well, but the way it produces the numbers in the table is otherwise simple and the difference in the Total Size Size Diff value and the sum of individual Size Diff values gives the size of the newly added metrics
There was a problem hiding this comment.
I replied in the email with division of sizes. Just pasting here but we can continue discussion in email thread.
| Source | Size(KB) |
|---|---|
| OdspPrefetchSnapshot(Main code file) | 2.72 |
| Common-utils | 3.86 |
| uuid | 1.14 |
| node-fetch | 0.325 |
| ms/index.js | 1.38 |
| node_modules/debug/src | 4.26 |
| Telemetry utils | 5.05 |
| odsp-doclib-utils | 3.5 |
| driver-utils | 4.16 |
| Driver-defn | 2.07 |
| 28.465 |
tylerbutler
added
area: examples
Fixes: #5171
This adds an api to fetch the latest snapshot and store it the persisted cache. Hosts can then use it to prefetch before loading the container so as to avoid the trees/latest network call from critical path as then the snapshot will be fetched from the cache.