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

Codesplit WebGL/WebGPU entrypoints #29404

Merged
merged 27 commits into from
Nov 8, 2024

Conversation

CodyJasonBennett
Copy link
Contributor

@CodyJasonBennett CodyJasonBennett commented Sep 13, 2024

Fixed #29156

Description

Emits a build where three/webgpu re-exports from core to avoid user configuration or duplication. Each entrypoint will no longer bundle a unique copy of three.js but share a single copy.

To prevent future issues with tree-shaking (#28670), I've code split the common core and code specific to a backend.

// src/Three.core.js -> build/three.core.js -> build/three.core.min.js
export { Vector3 } from './math/Vector3.js';

// window.__THREE__ check for duplication here
// src/Three.js -> build/three.module.js -> build/three.module.min.js -> build/three.cjs

export * from './Three.core.js';
export { WebGLRenderer } from './renderers/WebGLRenderer.js';
// src/Three.WebGPU.js -> build/three.webgpu.js -> build/three.webgpu.min.js

export * from './Three.core.js';
export { WebGPURenderer } from './renderers/webgpu/WebGPURenderer.js';
// src/Three.WebGPU.Nodes.js -> build/three.webgpu.nodes.js -> build/three.webgpu.nodes.min.js

export * from './Three.core.js';
export { WebGPURenderer } from './renderers/webgpu/WebGPURenderer.Nodes.js';

Copy link

github-actions bot commented Sep 13, 2024

📦 Bundle size

Full ESM build, minified and gzipped.

Before After Diff
WebGL 690.29
171.01
338.07
78.63
-352.22 kB
-92.38 kB
WebGPU 815.35
219.78
467.13
129.1
-348.22 kB
-90.68 kB
WebGPU Nodes 814.86
219.65
466.83
129.03
-348.03 kB
-90.62 kB

🌳 Bundle size after tree-shaking

Minimal build including a renderer, camera, empty scene, and dependencies.

Before After Diff
WebGL 463.31
111.89
463.4
111.57
+93 B
-314 B
WebGPU 538.09
145.28
538.09
145.28
+0 B
+0 B
WebGPU Nodes 494.2
135.15
494.2
135.15
+0 B
+0 B

input: 'test/treeshake/index.webgpu.nodes.js',
input: 'test/treeshake/index.webgpu.js',
Copy link
Contributor Author

@CodyJasonBennett CodyJasonBennett Sep 15, 2024

Choose a reason for hiding this comment

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

Undid this change, but three.webgpu.nodes.js is unused, and duplicate with WebGPU (missing only the recent BundleGroup export). CI still measures WebGL, WebGPU, WebGPU + Nodes. Maybe it can measure the common core instead?

@donmccurdy
Copy link
Collaborator

donmccurdy commented Sep 15, 2024

I think this looks like a good approach! Shared 'core' that the WebGL and WebGPU entrypoints can use, and that doesn't need to be exposed as a separate public entrypoint to users.

(obsolete / resolved issue)

Minor thing, but I'm seeing some duplication of build artifacts when running npm run build locally:

> ls ./build

three.cjs                 three.module.js           three.webgpu.min.js
three.core.min2.js        three.module.min.js       three.webgpu.nodes.js
three.core2.js            three.webgpu.js           three.webgpu.nodes.min.js

Copy link
Collaborator

@donmccurdy donmccurdy left a comment

Choose a reason for hiding this comment

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

I will vote for Cody's approach here, as it keeps both the WebGL and WebGPU entrypoints minimal even when used from a CDN.

Alternatively we could skip the shared 'three.core.js' file and have 'three.webgpu.js' import from 'three.module.js'. That's fine for the bundler users, but CDN users would get a lot of extra JavaScript when trying to use WebGPU. So that would presumably be more of a temporary stopgap.

Tested:

  • compared build sizes before/after in production builds
  • compared build sizes before/after in dev builds
  • no dual package hazard in production builds in an application w/ Vite
  • no dual package hazard in development builds in an application w/ Vite

@Mugen87 Mugen87 added this to the r170 milestone Oct 10, 2024
@CodyJasonBennett
Copy link
Contributor Author

Should I just remove the build artifacts from this PR or regenerate them?

@Mugen87
Copy link
Collaborator

Mugen87 commented Oct 12, 2024

Removing them is better 👍 .

@Mugen87
Copy link
Collaborator

Mugen87 commented Oct 13, 2024

@mrdoob @sunag Fixing #29156 is crucial for the community so if there are no objections from your side let's merge this so we have a bit of room to r170.

The only workflow that is affected by this change is when devs copy builds around since there is three.core.js now. However, this is not a workflow that we promote in our installation guide (1. npm/build tool, 2. CDN). So I think this is good to go.

@sunag
Copy link
Collaborator

sunag commented Oct 13, 2024

I like the changes 👍

@Mugen87
Copy link
Collaborator

Mugen87 commented Oct 15, 2024

Well, seems like we all agree to move on with this? Who's gonna take the lead and merge this (and #29644 next)?

@mrdoob should definitely approve this change. The type of builds and the import structure is a fundamental aspect of the library and I'm not sure how he feels about this approach.

I understand the PR is a great solution for our current promoted workflows (npm, CDN) but it moves away from the idea to have a renderer with its core classes in a single self-contained build file. Regarding #29644 I'm not sure it's a good user experience to have three, three/webgpu and three/tsl. Hopefully we can merge three/webgpu and three/tsl somehow so the imports get not too complicated for users.

@CodyJasonBennett
Copy link
Contributor Author

If you really need a compromise, we can bundle everything into the minified three.module.min.js and three.webgpu.min.js so they are self-contained, but continue code-splitting for non-minified bundles which NPM will accept. That assumes you'd only use one minified bundle at a time, as Renaud indicated for examples. I'm personally not a fan of example code or imports themselves diverging too much based on environment (CDN, bundler), but that is a casualty of this approach or effective workaround.

<script type="importmap">
	{
		"imports": {
			"three": "../build/three.webgpu.min.js"
		}
	}
</script>
import * as THREE from 'three';
const renderer = THREE.WebGPURenderer();
// npm i three
import * as THREE from 'three/webgpu';
const renderer = THREE.WebGPURenderer();

@Mugen87
Copy link
Collaborator

Mugen87 commented Oct 15, 2024

It seems the examples can't be properly debugged and inspected if the builds are minified. Ideally, they use an unminified bundle.

That said, I'm personally fine with the initial approach you have implemented. I'm just curious to see how @mrdoob evaluates the PR and the need for self-contained bundles.

@CodyJasonBennett
Copy link
Contributor Author

Is there a reason we don't emit sourcemaps? That would have errors in all bundles point back to source code and allow tracing in Lighthouse.

@donmccurdy
Copy link
Collaborator

@CodyJasonBennett just checking you're aware that the sourcemaps are currently built with yarn dev but not yarn build — Are you asking about building and publishing sourcemaps to npm as well?

@RenaudRohlinger
Copy link
Collaborator

@mrdoob should definitely approve this change. The type of builds and the import structure is a fundamental aspect of the library and I'm not sure how he feels about this approach.

That said, I'm personally fine with the initial approach you have implemented. I'm just curious to see how @mrdoob evaluates the PR and the need for self-contained bundles.

Gentle ping @mrdoob 😊. This PR has received approval from most collaborators, but before proceeding, we would really appreciate your input. The import structure and build types are critical to the library's architecture, and we'd like to understand your perspective on the approach:

Self contained:

// npm i three
import * as THREE from 'three/webgpu';
const renderer = THREE.WebGPURenderer();

Or not:

import * as THREE from 'three';
import { WebGPURenderer } from 'three/webgpu'

@mrdoob mrdoob modified the milestones: r170, r171 Oct 31, 2024
@trusktr
Copy link
Contributor

trusktr commented Nov 5, 2024

This update will still require people to update their build setups (or importmaps). The idea in #29156 (comment) would fully avoid the requirement for build setups to be updated, while providing optional ways to achieve the same goals as this new three/webgpu import path. That alternative would make ecosystem migration easier. Updating a version number and using new APIs is inevitable, but having to also update build setups is a hassle that could be avoided.

@CodyJasonBennett
Copy link
Contributor Author

I elaborated in #29156 (comment), but this is a gross misdiagnosis.

@mrdoob
Copy link
Owner

mrdoob commented Nov 6, 2024

The src code split sounds good to me 👌

The naming is becoming weird though.

This would look better to me:

src/Constants.js
src/Core.js
src/Legacy.js
src/Three.js // do we still need this one?
src/Utils.js
src/WebGL.js
src/WebGPU.js

The builds, I'm not so sure...

What do you guys think of this?

builds/three.core.js
builds/three.webgl.js
builds/three.webgpu.js
builds/three.module.js

three.core.js: Core
three.webgl.js: WebGLRenderer only
three.webgpu.js: WebGPURenderer only (And NodeMaterials only)
three.module.js: Core + WebGLRenderer + WebGPURenderer

@donmccurdy
Copy link
Collaborator

donmccurdy commented Nov 6, 2024

@mrdoob For src/ files, I like your suggestion. For build/ files, I would think of it in terms of package.json#exports entrypoints. It is probably too soon for the three entrypoint to include WebGPURenderer. Something like this I would support...

  • three -> Core + WebGLRenderer
  • three/webgpu -> Core + WebGPURenderer + Nodes

... which could be implemented on top of build/three.core.js, build/three.webgl.js, and build/three.webgpu.js bundles, similar to what's in this PR but renamed slightly. Optionally there could be additional three/core, three/webgl, and three/tsl entrypoints... I don't feel strongly about any of those others at the moment.

@RenaudRohlinger
Copy link
Collaborator

It is probably too soon for the three entrypoint to include WebGPURenderer. Something like this I would support...

Having used WebGPURenderer across all my projects for over a year, I fully agree with the concerns. Given the WebGPU API's ongoing instability, it seems premature to push this onto most Three.js developers, especially since many Three.js developers may not adopt WebGPURenderer for some time. Adding it now could undermine previous efforts to optimize bundle size through tree-shaking:

image

(see #29827)

A structure like this strikes a balanced approach:

three -> Core + WebGLRenderer
three/webgpu -> Core + WebGPURenderer + Nodes

This setup respects the longevity of WebGLRenderer, which is likely to remain in widespread use, while also providing a clear, separate path for WebGPURenderer. Merging both renderers into the same module would likely add unnecessary complexity, with WebGLRenderer and WebGLBackend coexisting in a confusing, overlapping space.

There’s no real benefit in bundling both WebGLRenderer and WebGPURenderer together in the same app, as they can’t communicate with each other. Furthermore simply switching WebGLRenderer to WebGPURenderer isn’t feasible either, given that the material systems are fundamentally different.

@sunag
Copy link
Collaborator

sunag commented Nov 7, 2024

I think the naming suggestion is a good one, as the example Core.js instead of Three.core.js.

Merging the two renderers into a single module could limit flexibility in the creation process. For example, in cases like PMREMGenerator, where we kept the same API/interface but internally moved from GLSL to TSL, this could pose challenges if we need to do it in other core classes. It may also make it more difficult to simply swap the renderer from WebGLRenderer to WebGPURenderer, along with other concerns already mentioned.

I would suggest moving forward with the renaming and merging the PR as proposed, and addressing this additional issue later if needed.

@Mugen87
Copy link
Collaborator

Mugen87 commented Nov 7, 2024

I would suggest moving forward with the renaming and merging the PR as proposed, and addressing this additional issue later if needed.

Absolutely. What has been suggested in #29404 (comment) sounds good to me as well.

@mrdoob
Copy link
Owner

mrdoob commented Nov 7, 2024

#29404 (comment) sounds good to me too 👍

@RenaudRohlinger
Copy link
Collaborator

Nice 🚀! Then I think we can merge this PR as it is? /cc @Mugen87

@Mugen87
Copy link
Collaborator

Mugen87 commented Nov 8, 2024

Yeah, let's start with this configuration. We can apply further updates with additional PRs if necessary.

@Mugen87 Mugen87 merged commit 84a69f5 into mrdoob:dev Nov 8, 2024
12 checks passed
@mrdoob
Copy link
Owner

mrdoob commented Nov 20, 2024

📦 Bundle size

Full ESM build, minified and gzipped.

Before After Diff
WebGL 690.29
171.01
338.07
78.63
-352.22 kB
-92.38 kB

Hmm, how come the minified build is now -92.38 kB less? 🤔

@Mugen87
Copy link
Collaborator

Mugen87 commented Nov 20, 2024

I could imagine the script still measures the sizes based on a single build file and does not honor three.core so far.

@mrdoob
Copy link
Owner

mrdoob commented Nov 22, 2024

Ah, got it. I'll need to update that table again then...

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.

Provide a WebGPU build that re-exports from three
7 participants