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

Revert "Revert "Revert closure optimisations"" #12348

Closed
wants to merge 1 commit into from
Closed

Revert "Revert "Revert closure optimisations"" #12348

wants to merge 1 commit into from

Conversation

chazcb
Copy link
Contributor

@chazcb chazcb commented Oct 6, 2017

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.

// jest test file

import Matrix4 from 'three/src/math/Matrix4'
// raises "TypeError: _Vector.Vector3 is not a constructor"

@Itee
Copy link
Contributor

Itee commented Oct 6, 2017

To remind: #10547, #10627, #11226, #11246

@mrdoob
@Mugen87
@drcmda
@pailhead
@zorro-fr24

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 minor MAJOR...
In my world: https://jsperf.com/test-identity-check-cost/5
For me under chrome 61.0.3163.100 _ 64bits, i got:

  • without check 1514 +/- 2.57% (fastest)
  • with OR check 385 +/-0.26% (74% slower)
  • with identity check 389 +/- 0.50% (74% slower) and voila !

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 ?
So first of all, could you show the incriminated piece of code (I would try to reproduce, please) ? Next, why would you use this test library instead of the currently implemented QUnit ? It is for your own code base, or to replace QUnit ? Did you ask Jest team about this problem, maybe they had the same problem than webpack ?

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,
Tristan

@chazcb
Copy link
Contributor Author

chazcb commented Oct 6, 2017

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 foo the perf gain is much lower.

@chazcb
Copy link
Contributor Author

chazcb commented Oct 6, 2017

@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

@Itee
Copy link
Contributor

Itee commented Oct 6, 2017

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 ?

@Itee
Copy link
Contributor

Itee commented Oct 6, 2017

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 !

@chazcb
Copy link
Contributor Author

chazcb commented Oct 6, 2017

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).

@chazcb
Copy link
Contributor Author

chazcb commented Oct 6, 2017

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 (web and closured) but I added them for clarity. babel is not so obvious that it should fail, but it does because babel's current behavior is to turn modules into commonjs type modules (I believe). node demos current default Node behavior, and finally hoisted demos the behavior we're getting w/ three's rollup. I can flip all these demos over to use real three.js code, but I'm hoping this is clear enough to further the discussion.

@chazcb
Copy link
Contributor Author

chazcb commented Oct 6, 2017

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.

@chazcb chazcb closed this Oct 6, 2017
@chazcb
Copy link
Contributor Author

chazcb commented Oct 12, 2017

@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

@Itee
Copy link
Contributor

Itee commented Oct 13, 2017

I will take a look

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.

2 participants