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

proof of concept: assets as references to blobs in indexedDB, not directly as base64 in store #3745

Closed
wants to merge 1 commit into from

Conversation

mimecuvalo
Copy link
Collaborator

@mimecuvalo mimecuvalo commented May 13, 2024

As I look at LOD holistically and whether we have multiple sources when working locally, I learned that our system used base64 encoding of assets directly. Issue #3728

assetstore

The motivations and benefits are:

  • store size: not having a huge base64 blobs injected in room data
  • perf on loading snapshot: this helps with loading the room data more quickly
  • multiple sources: furthermore, if we do decide to have multiple sources locally (for each asset), then we won't get a multiplicative effect of even larger JSON blobs that have lots of base64 data in them
  • encoding/decoding perf: this also saves the (slow) step of having to base64 encode/decode our assets, we can just strictly with work with blobs.

The benefit here is clear! I created an AssetBlobObjectStore which probably needs some thought (again, this is proof-of-concept), so if we decide to move forward, let's discuss how best to integrate that part.

Todo:

  • decodes video and images
  • make sure it syncs to other tabs
  • make sure it syncs to other multiplayer room
  • fix tests
  • have backup for existing editors instantiated without assetBlobStore

Change Type

  • sdk — Changes the tldraw SDK
  • dotcom — Changes the tldraw.com web app
  • docs — Changes to the documentation, examples, or templates.
  • vs code — Changes to the vscode plugin
  • internal — Does not affect user-facing stuff
  • bugfix — Bug fix
  • feature — New feature
  • improvement — Improving existing features
  • chore — Updating dependencies, other boring stuff
  • galaxy brain — Architectural changes
  • tests — Changes to any test code
  • tools — Changes to infrastructure, CI, internal scripts, debugging tools, etc.
  • dunno — I don't know

Test Plan

  1. Test the shit out of uploading/downloading video/image assets, locally+multiplayer.
  • Need to fix current tests and write new ones

Release Notes

  • Assets: store as reference to blob in indexedDB instead of storing directly as base64 in the snapshot.

Copy link

vercel bot commented May 13, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
examples ❌ Failed (Inspect) May 13, 2024 8:32am
1 Ignored Deployment
Name Status Preview Updated (UTC)
tldraw-docs ⬜️ Ignored (Inspect) May 13, 2024 8:32am

@huppy-bot huppy-bot bot added improvement Improves existing features sdk Affects the tldraw sdk labels May 13, 2024
@@ -241,7 +241,7 @@ async function getRoomData(
// processed it
if (!asset) continue

data[asset.id] = await cloneAssetForShare(asset, uploadFileToAsset)
data[asset.id] = await cloneAssetForShare(asset, editor, uploadFileToAsset)
Copy link
Collaborator

Choose a reason for hiding this comment

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

secret style guide nit: whenever editor is an argument to a function, let's make it the first argument

*/
async getAssetBlobFromObjectStore(asset: TLAsset): Promise<Blob | undefined> {
return await this.assetBlobCache.get(asset, () =>
this.assetBlobStore.getAssetBlobFromObjectStore({ asset })
Copy link
Collaborator

Choose a reason for hiding this comment

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

TIL our weakmap caches can work with async stuff

@SomeHats
Copy link
Contributor

left some other thoughts on this here: #3764 (comment)

my take on this is that I like what we're doing with this, but i'm not sure it belongs quite so core as this makes it. i think blob storage is quite niche - it's really only relevant if you have an editor saving locally in the browser, whereas most people either have a multiplayer room or some sort of other saving mechanism. i certainly don't think it should be the default as this PR makes it.

instead, i would love to see a slightly more general system for making asset storage/retrieval more flexible. what would it look like for this to be build entirely in userland on your asset rewriting hook from #3764, for example?

@mimecuvalo
Copy link
Collaborator Author

my take on this is that I like what we're doing with this, but i'm not sure it belongs quite so core as this makes it. i think blob storage is quite niche - it's really only relevant if you have an editor saving locally in the browser, whereas most people either have a multiplayer room or some sort of other saving mechanism. i certainly don't think it should be the default as this PR makes it.

yeah, totally fair to not make it core, definitely would make sense more as a sidecar kind of option.

instead, i would love to see a slightly more general system for making asset storage/retrieval more flexible. what would it look like for this to be build entirely in userland on your asset rewriting hook from #3764, for example?

i see the overlap for sure, but i think in my head they're quite different (as far as storage vs. dynamic retrieval). i'd avoid making a system that tries to address them both perhaps because they're trying to address two different problems. (it's maybe a coincidence that I'm working on both at the same time heh)

let's discuss more tomorrow!

@mimecuvalo
Copy link
Collaborator Author

(closed in favor of #3836)

@mimecuvalo mimecuvalo closed this May 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improves existing features sdk Affects the tldraw sdk
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants