-
-
Notifications
You must be signed in to change notification settings - Fork 35.5k
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
Revert "Revert "Revert closure optimisations"" #12348
Conversation
This reverts commit b76fff2.
To remind: #10547, #10627, #11226, #11246 @mrdoob First of all, i'm really sorry to being late on this topic... 2 works at same time, 3 children, 1 house in construction... the life ! So all my apologies. And nice too see you again ! Ok ok i readed the topix #11333, and just some little things... First of all, all these optimizations are here to avoid some circular references inside three.js with a gain of performance. If we revert all the things to the primary state, circular references will come back too, isn't it ? And performance will decrease too. You said that the performance gain is
I understand too that the problem is fixed for webpack 2.5.1 users, right ? So, if i fully understand your problem... You got circular reference for Vec3 and Mat4 due to the import inside Jest, right ? I hope we will find a better answer than revert all these things, like a include check on top of circular files maybe ?! Best regards, |
The reason you're getting the high performance benefit is because your for loop does not have any other changing code, so Chrome optimizes the entire for loop in the VM. If you do any sort of changing operation on |
@TristanVALCKE yeah, definitely the reason is because we use three.js in a larger code base. Our code base is tested using jest, which evaluates modules individually (e.g. it will evaluate Vector3 in isolation, then attempt to find all of Vector3's dependencies - in this case Matrix4). The issue is only fixed if you are using webpack, which is great, because we use webpack when bundling for production! But it still makes testing any three.js related code not possible for us. I'll try to get a Jest suite up and running that shows the issue, but in the meanwhile, some example code: This code works because Vector3 and Matrix4 functions are hoisted to the top of the evaluation. function Vector3() {}
Vector3.propotype = { // or using Object.assign
unproject: function () {
var matrix = new Matrix4()
return function unproject() { ... }
}()
}
function Matrix4() {}
Matrix4.propotype = { // or using Object.assign
extractRotation: (function () {
var vector3 = new Vector3()
return function extractRotation() { ... }
})()
} This code fails because the constructor functions are only hoisted within their local module closure. // ./Vector3.js
const Matrix4 = require('Matrix4')
function Vector3() {}
Vector3.propotype = { // or using Object.assign
unproject: function () {
var matrix = new Matrix4()
return function unproject() { ... }
}()
}
export default Vector3 // ./Matrix4.js
const Vector3 = require('Vector3')
function Matrix4() {}
Matrix4.propotype = { // or using Object.assign
extractRotation: (function () {
var vector3 = new Vector3()
return function extractRotation() { ... }
})()
}
export default Matrix4 |
Ok thank for fast reply ! That what i understood... what class are affected by this problem ? Only Vector3 and Matrix4 or there is lot of other incriminated class ? And what about Jest team about circular references ? |
In my opinion, circular reference is not an edge case in javascript library and Jest will need ( a day ) to deal with it... So maybe they currently have a solution to solve this problem. About the test: https://jsperf.com/test-identity-check-cost/7 I got 10% slower which is more acceptable ! |
I'm putting together a repo right now to demo the problem with both babel's default module loader and w/ webpack. In the meanwhile, yeah, I don't think the issue is circular dependency (this revert doesn't actually resolve a circular dep issue, it just avoids using constructors which are not hoisted). |
Alright @TristanVALCKE added a repo here https://github.com/chazcb/circular-deps-es6 that has a few different versions of module loaders. Two are intended to obviously fail ( |
Welp, turns out I can also subvert Jest to use the fully built three.module.js file (with hoisting) instead of the individual source files. I'll try that for now since it fixes our immediate issue. |
@mrdoob @TristanVALCKE just FYI, I think that Ashi is referencing this same issue here in her talk https://youtu.be/DJ-oSy0tN_U?t=884 cc @queerviolet would love if you chimed in here |
I will take a look |
This reverts commit b76fff2.
Related to conversations here #11333
More context for our use case: currently Three.js relies on the hoisting of function declarations to avoid some circular dependencies. For instance, in the built three.module.js file, Vector3 and Matrix4 function constructors evaluated before their prototypes are constructed because of hoisting. However, when we use a module loader like webpack or Jest's default loader to grab the Vector3 directly, the constructor are only hoisted to the tops of their own files, which results in Vector3 or Matrix4 files failing to evaluate.