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

Replace FinalizationRegistry based Texture Usage GC with Manual Strategy #27

Merged
merged 7 commits into from
Oct 6, 2023

Conversation

frank-weindel
Copy link
Collaborator

@frank-weindel frank-weindel commented Oct 3, 2023

  • Usaged-Based Texture Memory management now defaults to a new manual reference count and time threshold strategy.
    • This removes the requirement of the FinalizationRegistry browser feature which is not available prior to chrome 84 / WPE 2.38.
    • The prior behavior is still available by setting a new experimental renderer setting to true:
      • experimental_FinalizationRegistryTextureUsageTracker
  • Added texture-memory-stress test with instructions on how to perform.
  • Example tests may now export a customSettings function which returns any renderer settings overrides that the test requires to run. These settings are applied on top of all of the defaults specified by the example test launcher.
  • Improve type checking for EffectDesc
    • type now determines the specific properties types checked.
  • Breaking changes:
    • Refactor ShaderDesc / TextureDesc related areas including change their names to ShaderRef / TextureRef.
      • RendererMain.makeTexture renamed to RendererMain.createTexture
      • RendererMain.makeShader renamed to RendererMain.createShader
      • TextureDesc type renamed to TextureRef type
      • ShaderDesc type renamed to ShaderRef type

Breaking changes:
- RendererMain.makeTexture -> RendererMain.createTexture
- RendererMain.makeShader -> RendererMain.createShader
- TextureDesc type -> TextureRef type
- ShaderDesc type -> ShaderRef type
@frank-weindel frank-weindel added the do not merge Do not merge this PR. It may not be ready or depends on another PR to be merged first. label Oct 3, 2023
@frank-weindel frank-weindel changed the title Replace FinalizationRegistry based Texture Usage GC with Manual Strategy Replace FinalizationRegistry based Texture Usage GC with Manual Strategy (WIP) Oct 3, 2023
@frank-weindel
Copy link
Collaborator Author

@erikhaandrikman Could I get your thoughts on this approach?

@frank-weindel frank-weindel removed the do not merge Do not merge this PR. It may not be ready or depends on another PR to be merged first. label Oct 4, 2023
@frank-weindel frank-weindel changed the title Replace FinalizationRegistry based Texture Usage GC with Manual Strategy (WIP) Replace FinalizationRegistry based Texture Usage GC with Manual Strategy Oct 4, 2023
@wouterlucas wouterlucas linked an issue Oct 5, 2023 that may be closed by this pull request
@wouterlucas
Copy link
Contributor

Would there ever be a case where I'd want to run ThreadX without FinalizationRegistry?

@frank-weindel
Copy link
Collaborator Author

frank-weindel commented Oct 5, 2023

Would there ever be a case where I'd want to run ThreadX without FinalizationRegistry?

@wouterlucas FinalizationRegistry for texture memory isn't really coupled to ThreadX at all. It's just one strategy for proactively cleaning up textures that we've cached and loaded on the GPU that we are pretty confident won't be needed again for awhile. In the case of the FinalizationRegistry strategy we know 100% that a texture is no longer referenced anywhere in the currently running JavaScript at all. The new Manual Count + Interval strategy employs a time heuristic that aims to predict when that is the case, but with some chances of being wrong. There are pros/cons to both approaches. GC and the firing of the FinalizationRegistry events for instance isn't guaranteed in any particular timeframe by the browser. It could come super late, for instance, to a point where it's not being proactive enough to prevent memory pressure overload. The Manual Count strategy is guaranteed to run and clean up unused textures regularly.

@frank-weindel frank-weindel merged commit b9a3563 into main Oct 6, 2023
1 check passed
@frank-weindel frank-weindel deleted the texture-management-replace-finalizationregistry branch October 6, 2023 02:21
chiefcll pushed a commit that referenced this pull request Oct 20, 2023
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.

Support TV Devices
3 participants