-- size, ++ speed #8824

tschw · 2016-05-06T11:11:28Z

This PR adds a custom build step that replaces THREE & WebGL symbolic constants with numerals. It also contains some minor cleanup based on diagnostics from the JS build.

The build step is implemented both in Python and in JavaScript for both build variants. It has a debug mode that logs the replacements.

tschw · 2016-05-06T11:24:47Z

Related: #8822

gero3 · 2016-05-06T11:26:47Z

The package.json in build should be deprecated and replaced with the one in the root. It is no longer useful to have it in there.

tschw · 2016-05-06T11:32:59Z

@gero3

The package.json in build should be deprecated and replaced with the one in the root. It is no longer useful to have it in there.

Ah! What is a smooth way to deprecate it?

Is build.min a good name for the minifying build? Is there some established standard we can follow, maybe?

tschw · 2016-05-06T11:38:31Z

@gero3
Does the root package.json run on Windows? Seeing cd ./utils/build && node ... I guess it might assume a UNIX-style shell (although I'm not sure what Windows can handle these days).

If so, can we just invoke the build from the root directory, maybe?

gero3 · 2016-05-06T11:42:32Z

I like the idea of the constifier but not the implementation right now. Mostly because it requires us to keep track of constants in 2 places now.

@gero3

The package.json in build should be deprecated and replaced with the one in the root. It is no longer useful to have it in there.
Ah! What is a smooth way to deprecate it?

Just delete the file. It is just a relic now. Nobody should be using it anyway. It isn't kept up to date anyways.

Is build.min a good name for the minifying build? Is there some established standard we can follow, maybe?

I actually would add the following actually. That way it is similar to the python version.

script: {
  build: "npm run build.debug && npm run build.release",
  build.debug: "cd ./utils/build && node build.js --include common --include extras --output ../../build/three.js",
  build.release: "cd ./utils/build && node build.js --include common --include extras --minify --output ../../build/three.min.js"
}

gero3 · 2016-05-06T11:56:27Z

@gero3
Does the root package.json run on Windows? Seeing cd ./utils/build && node ... I guess it might assume a UNIX-style shell (although I'm not sure what Windows can handle these days).

If so, can we just invoke the build from the root directory, maybe?

Yes, it also runs on WIndows. The build script is made to run specifically from the directory, similar to the python script.

gero3 · 2016-05-06T11:59:51Z

@tschw
I don't get why you changed MaterialIdCount to materialIdCount.

tschw · 2016-05-06T12:27:25Z

I don't get why you changed MaterialIdCount to materialIdCount.

I like the idea of the constifier but not the implementation right now. Mostly because it requires us to keep track of constants in 2 places now.

Because it doesn't.

See utils/build/constifier/dumper.html extracts all uppercase names on THREE and a GL context that have the type number. It then outputs the JSON file. To update simply copy&paste.

tschw · 2016-05-06T12:39:16Z

@gero3

I don't get why you changed MaterialIdCount to materialIdCount.

Also, these names are global variables, not constants. So they should follow the conventions for variables not constants.

We could put some deprecation code into Three.Legacy.js or, alternatively, if we really want them to be inconsistently uppercased, initialize them to null or blacklist them.

@tschw wrote:

See utils/build/constifier/dumper.html ...

I guess it might be possible to automate it even further, using this tool only only for hard-wiring the WebGL constants, extracting the THREE constants via node.js. Is it possible to load THREE in this context? I guess you may have some experience?

gero3 · 2016-05-06T13:10:13Z

@tschw wrote:

See utils/build/constifier/dumper.html ...
I guess it might be possible to automate it even further, using this tool only only for hard-wiring the WebGL constants, extracting the THREE constants via node.js. Is it possible to load THREE in this context? I guess you may have some experience?

Using Eval the concatenated source code could get you the the THREE object which can be used in the findConstants object.

tschw · 2016-05-06T13:17:09Z

@gero3

Using Eval the concatenated source code could get you the the THREE object which can be used in the findConstants object.

Would it be a (better) option to just require the temp file? Does Three.js set module.exports when module is provided, that is?

gero3 · 2016-05-06T13:21:06Z

It does. But you can't require a string without writing it to the disk somewhere.

tschw · 2016-05-06T13:41:34Z

@gero3

It does.

Ah cool!

But you can't require a string without writing it to the disk somewhere.

Right - it's only in memory with the JS build... Isn't eval deprecated? Also, extracting the constants every time would slow down the build quite a bit, which might be acceptable since we're talking release builds only. I don't think writing a temp file would dominate.

Maybe constant extraction should just be a separate invocation that performs a debug build as a prerequisite?

@mrdoob
What are your thoughts on the *IdCount considerations above?

Yet another option (that would allow them to remain uppercased) would be to move them onto the classes, e.g. Material.InstanceId. As a matter of personal taste, I'd like it better if this kind of naming would indicate something that does not change, however, I think it should be your call.

gero3 · 2016-05-06T14:05:51Z

uglify is very slow compared to the extraction process normally. It shouldn't take a second to get all of the constants.

bhouston · 2016-05-06T15:41:13Z

Why don't we just adopt JavaScript symbols which are designed for this?

https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/Symbol

Constantly like this do not really impact ThreeJS's size at all because they get reduced by gzip. This seems like a special case hard to maintain gzip step that isn't necessary. I would like to see the differences in resulting gzip size before and after this PR. Is it significantly better?

I've done my own experiments before with changing THREE to T and Vector3 to V3 and it made almost no difference in the resulting gzip file because well compression.

I do not understand the point of this PR.

Also this PR changes the static variable naming convention from first uppercase letter to lowercase for a number of constants. I am okay with changing convention but as its own PR.

bhouston · 2016-05-06T15:42:37Z

@gero3 wrote:

The package.json in build should be deprecated and replaced with the one in the root. It is no longer useful to have it in there.

I completely agree. :)

bhouston · 2016-05-06T15:52:55Z

Ah! What is a smooth way to deprecate it?

Just kill it, it doesn't work well and I doubt many are using it.

tschw · 2016-05-06T15:53:39Z

@bhouston

Symbols...

...won't make the properties constant. Only the data. Also, they are unsupported by most browsers.

Constantly like this do not really impact ThreeJS's size at all because they get reduced by gzip. This seems like a special case hard to maintain gzip step that isn't necessary.

There is no gzip step.

I would like to see the differences in resulting gzip size before and after this PR. Is it significantly better?

Just see above. It's 2KB gzipped and that equates to a lot of code...
I also see a ~ 10% JS speedup with Firefox.

bhouston · 2016-05-06T15:57:39Z

There is no gzip step.

Most webservers serve *.js files (and other text files) as gzipped (or soon brotli) automatically, at least in professional contexts. https://en.wikipedia.org/wiki/HTTP_compression Many webservers, like nginx, and do this transparently and cache the gzipped intermediates automatically: http://nginx.org/en/docs/http/ngx_http_gzip_module.html

tschw · 2016-05-06T15:58:56Z

@bhouston
I know. And I measured 2KB difference piping the resulting three.min.js file through gzip.

tschw · 2016-05-06T16:00:24Z

@bhouston

I've done my own experiments before with changing THREE to T and Vector3 to V3 and it made almost no difference in the resulting gzip file because well compression.

Those occur much more frequently, therefore compress to almost nothing anyway.

bhouston · 2016-05-06T16:04:56Z

2K, that is sort of what I saw as well, that is 2% saving, which isn't much. This to me isn't a major issue.

I am not sure that is sufficient to obfuscate the source files like this and duplicate the constants in two places which increases the maintenance complexity of Three.js. I really do not like violating DRY just to save 2K: https://en.wikipedia.org/wiki/Don%27t_repeat_yourself for just a 2K saving.

There are higher priority issues with ThreeJs in my opinion.

bhouston · 2016-05-06T16:07:38Z

I slightly oppose this for now because of the DRY violation. But if that was addressed, I wouldn't have much of an opinion on this PR either way, the savings are minimal, but the build step can easily be turned off is needed, so it sort of nets out to even.

tschw · 2016-05-06T16:10:32Z

I slightly oppose this for now because of the DRY violation.

Please read the full thread before opposing because there is none. Constant extraction was automated right from the start and I'm currently automating it further. It will be completely transparent.

tschw · 2016-05-06T16:13:24Z

@bhouston
Also using symbolic constants like we do inhibits optimizations, causes switch..case to deoptimize on Chrome, and makes the renderer slower (this is particularly true for the standardized WebGL constants which are eliminated). It also complicates the architecture because you have to pass gl around, just to access constants on it - so we may even want to generate a debug mode file with those constants to decouple access to them from the live context.

tschw · 2016-05-06T16:15:44Z

@bhouston

2K, that is sort of what I saw as well, that is 2% saving, which isn't much.

2K gzipped actually equates to a lot of code!

abelnation · 2016-05-06T16:22:32Z

RE the cd utils/build step before running the build script, I agree that it would be more ideal if the build script could be run from any path. The script can detect it's own path via __dirname and then resolve all other paths relative to that.

see: https://github.com/abelnation/three.js/blob/improve-npm-build/utils/build/build.js#L8

bhouston · 2016-05-06T16:28:14Z

If other tools use the constified version and try to use THREE.XXXXXX what happens? Do they have to run this script on all of their code too? Do the examples work with this new version without changes?

tschw · 2016-05-06T16:32:01Z

@bhouston

If other tools use the constified version and try to use THREE.XXXXXX what happens? Do they have to run this script on all of their code too? Do the examples work with this new version without changes?

THREE.XXXX all exist. So external code will of course keep working. You can use it, if you like to.

It's also safe to use code like

THREE.XXXX = 111;

and the token will not be replaced in this case.

tschw · 2016-05-06T16:36:11Z

Do the examples work with this new version without changes?

I wouldn't even dare to make this a PR if they didn't :)

mrdoob · 2016-05-07T16:54:39Z

Would it be possible to split this PR in two? I would like to tackle the Object.assign() changes first and the constants later on.

- Without it 'npm install' issues a warning message.

- Replaces THREE and WebGL symbolic constants with numerals. - Reduces the build size by (currently) 8KB / 2KB gzipped. - Improves the performance of the renderer's JS code.

tschw · 2016-05-07T17:24:52Z

Would it be possible to split this PR in two? I would like to tackle the Object.assign() changes first and the constants later on.

Sure.

Thought at first this one broke, but then I looked at the build and everything was looking fine. Turns out I was in fact just testing canvas / cube believing it was webgl / cube :).

mrdoob · 2016-05-07T17:27:30Z

Thought at first this one broke, but then I looked at the build and everything was looking fine. Turns out I was in fact just testing canvas / cube believing it was webgl / cube :).

Haha!

tschw · 2016-05-08T04:03:24Z

@mrdoob
The threads grow long real quickly and I guess you can't always read it all. However, I figure (from upvoting above question) that you may not like that *IdCount renamer in this PR. If so, please help me pick one of the many alternatives there are. These two posts list the ones I could think of:

#8824 (comment)
#8824 (comment)

mrdoob · 2016-05-10T22:47:54Z

The threads grow long real quickly and I guess you can't always read it all. However, I figure (from upvoting above question) that you may not like that *IdCount renamer in this PR.

Actually, I was thinking we could make these counters internal. Like...

THREE.Texture = ( function () {
    var id = 0;
    return function () {
        Object.defineProperty( this, 'id', { value: id ++ } );
    };
} )();

This should work fine with Object3D, Material and Texture. However, BufferGeometry currently uses the same counter Geometry uses and that'll create issues inside WebGLRenderer...

Also, I wonder if people relies on THREE.*IdCount for something...

tschw · 2016-05-10T23:16:36Z

@mrdoob

However, BufferGeometry currently uses the same counter Geometry uses

How about Geometry.id (static), in that case? Or applying this pattern consistently?

Also, I wonder if people relies on THREE.*IdCount for something...

The latter would keep it exposed and allow deprecation of the old name via Three.Legacy, but maybe it's exotic enough to just solve it by documenting it for the next release on the Migration page?

tschw · 2016-05-16T22:58:47Z

/ping @mrdoob
What about this one?

mrdoob · 2016-05-17T00:05:05Z

I like the idea, but the *IdCount issue has become a bit of a blocker 😕

Rather than having to notify the user that these variables have changed name, I think I would like to remove them (using the approach in #8824 (comment)) and solve the BufferGeometry/Geometry/DirectGeometry collision.

Then the PR/Feature should be ready to go.

danrossi · 2017-11-30T11:47:40Z

Hi I believe this is the best thread to be commenting on. How is this coming along to reduce the filesize ? Is there a way to remove all the shaders not in use at all ?

donmccurdy · 2017-12-05T22:13:44Z

@danrossi More recently three.js has moved to ES6 modules and Rollup for its build (#9310). This means that for the particular case of pruning parts of three.js that aren't needed for your particular project, you can use tree-shaking in build systems like Rollup or Webpack. Here is an example of how to set that up: https://stackoverflow.com/questions/41538767/how-do-i-tree-shake-three-js-using-webpack-or-rollup

As far as efforts to reduce the "default" size of threejs, I don't current status of this PR.

danrossi · 2017-12-06T03:01:21Z

Ok not a problem I might have to make another ticket. for my purpose the bundle has to stay global and so not packaged in. Yes I use such imports but it's global.

I have a special build of three which only exports the required files. I can reduce it down to 350kb or there abouts.

However there is alot of things that are still included unnecessarily like all the shaders and their respective uniforms configs.

The shaders need to be modular somehow. I don't need any of them apart for MeshBasicMaterial and create my own with a RawShaderMaterial.

Not sure where to ask about that.

danrossi · 2017-12-06T18:05:08Z

I figured out a hack here to prevent including all the shader code. In the rollup build its trying to transform them I just provide an empty string.

function glsl() {

	return {

		transform( code, id ) {

			if ( /\.glsl$/.test( id ) === false ) return;

			if (!/meshbasic/.test(id)) {
				return {
					code: 'export default " "',
					map: { mappings: '' }
				};
			}

			var transformedCode = 'export default ' + JSON.stringify(
					code
						.replace( /[ \t]*\/\/.*\n/g, '' )
						.replace( /[ \t]*\/\*[\s\S]*?\*\//g, '' )
						.replace( /\n{2,}/g, '\n' )
				) + ';';

			return {
				code: transformedCode,
				map: { mappings: '' }
			};

		}

	};

}

danrossi · 2017-12-06T18:12:12Z

This code has proven to be a problem for an external project I am working on. I think this importing causes more problems than good. It is expecting three installed via npm. I might have to convert them all back to global prefixes. I can't just tell that code to treat three as global. Even though I am bundling that three code beforehand.

I am trying to include the module of that project into a custom three bundle and its trying to include duplicated files and increase the filesize. I think it has even included minified sources from somewhere.

Without that project included and with the shaders now removed I have managed to trim it down to 260kb !

import {
    //MeshBasicMaterial,
    RawShaderMaterial,
    BoxBufferGeometry,
    Mesh,
    Group,
    LinearMipMapLinearFilter,
    LinearFilter,
    DoubleSide
} from 'three';

danrossi · 2017-12-08T04:06:16Z

I am using import constants for a custom shader with morph target requires. So I have to include these back into the bundle. All other shaders can go for my needs. It still setups up a heap of helper properties in the ShaderLib. So extra code not needed.

if (!/morphtarget|begin_vertex|project_vertex/.test(id)) {
				return {
					code: 'export default " "',
					map: { mappings: '' }
				};
			}

danrossi · 2017-12-08T04:11:32Z

Please refer to this discourse in regards to one way tree shaking problems.

The import system is only designed to bundle three.js into an app. You can use the elegant import setup in a 3rd party module build and set three as global in the rollup config.

But when trying to bundle that module back into a custom three bundle you can't. The import code is tree shaking duplicated three module code for no reason and increases the size about 3 times. I think it even includes minified code.

For bundled my custom modules I have to hard code locations of the sources. I am using three github sources not npm. And don't even want it to look for npm three.

Sorry I don't know where else to report this.

https://discourse.threejs.org/t/tree-shaking-three-js/1349/17?u=danrossi

mrdoob · 2017-12-28T03:03:32Z

2K, that is sort of what I saw as well, that is 2% saving, which isn't much. This to me isn't a major issue.

👍

tschw added 8 commits May 7, 2016 19:04

Renamed global *IdCount variables to lowercase.

336dbfe

NPM build: Added SPDX compliant license string.

bfbfb62

- Without it 'npm install' issues a warning message.

Animation: Removed unused local variables.

3b00e2b

Renderer: Free temporary arrays on 'dispose'.

0676c43

NPM build: Add build.js invocation to package.json.

c5bfefb

Added "constifier" build step.

de6180e

- Replaces THREE and WebGL symbolic constants with numerals. - Reduces the build size by (currently) 8KB / 2KB gzipped. - Improves the performance of the renderer's JS code.

Automated extraction of THREE constants.

066af7f

Added 'build.release' to root folder package.json.

e9f738f

tschw force-pushed the Constants branch 2 times, most recently from d8d5d06 to 219b579 Compare May 7, 2016 17:12

tschw mentioned this pull request May 7, 2016

Use Object.assign for constant definitions. #8838

Merged

tschw force-pushed the Constants branch from 219b579 to e9f738f Compare May 7, 2016 17:19

Updated three.constspec.json to current build.

6fa4459

mrdoob closed this Dec 28, 2017

-- size, ++ speed #8824

-- size, ++ speed #8824

Conversation

tschw commented May 6, 2016 • edited Loading

tschw commented May 6, 2016

gero3 commented May 6, 2016

tschw commented May 6, 2016 • edited Loading

tschw commented May 6, 2016 • edited Loading

gero3 commented May 6, 2016

gero3 commented May 6, 2016 • edited Loading

gero3 commented May 6, 2016

tschw commented May 6, 2016 • edited Loading

tschw commented May 6, 2016 • edited Loading

gero3 commented May 6, 2016

tschw commented May 6, 2016

gero3 commented May 6, 2016

tschw commented May 6, 2016

gero3 commented May 6, 2016

bhouston commented May 6, 2016 • edited Loading

bhouston commented May 6, 2016

bhouston commented May 6, 2016

tschw commented May 6, 2016

bhouston commented May 6, 2016

tschw commented May 6, 2016

tschw commented May 6, 2016

bhouston commented May 6, 2016

bhouston commented May 6, 2016

tschw commented May 6, 2016

tschw commented May 6, 2016

tschw commented May 6, 2016

abelnation commented May 6, 2016

bhouston commented May 6, 2016

tschw commented May 6, 2016

tschw commented May 6, 2016

mrdoob commented May 7, 2016

tschw commented May 7, 2016

mrdoob commented May 7, 2016

tschw commented May 8, 2016

mrdoob commented May 10, 2016

tschw commented May 10, 2016

tschw commented May 16, 2016

mrdoob commented May 17, 2016

danrossi commented Nov 30, 2017 • edited Loading

donmccurdy commented Dec 5, 2017

danrossi commented Dec 6, 2017

danrossi commented Dec 6, 2017 • edited by mrdoob Loading

danrossi commented Dec 6, 2017

danrossi commented Dec 8, 2017

danrossi commented Dec 8, 2017

mrdoob commented Dec 28, 2017

tschw commented May 6, 2016 •

edited

Loading

tschw commented May 6, 2016 •

edited

Loading

tschw commented May 6, 2016 •

edited

Loading

gero3 commented May 6, 2016 •

edited

Loading

tschw commented May 6, 2016 •

edited

Loading

tschw commented May 6, 2016 •

edited

Loading

bhouston commented May 6, 2016 •

edited

Loading

danrossi commented Nov 30, 2017 •

edited

Loading

danrossi commented Dec 6, 2017 •

edited by mrdoob

Loading