Implement caching in the JS version #138
Conversation
|
|
||
| layoutNodeImpl(node, parentMaxWidth, parentDirection); | ||
|
|
||
| for (var key in node.layout) |
There was a problem hiding this comment.
Nit: please always use { } even for one lines
There was a problem hiding this comment.
@vjeux - this should probably be added to the static code analysis as part of the CI build. I'll look into it.
There was a problem hiding this comment.
Wow, just checked and the eslint files only has one rule! That definitely needs fixing ;-)
| @@ -1068,8 +1068,32 @@ var computeLayout = (function() { | |||
| child.nextAbsoluteChild = null; | |||
| } | |||
| } | |||
|
|
|||
| function layoutNode(node, parentMaxWidth, parentDirection) { | |||
There was a problem hiding this comment.
Would you mind porting the caching implementation to be closer to the C version this way it won't diverge in subtle ways
https://github.com/facebook/css-layout/blob/master/src/Layout.c#L1205-L1235
|
This is awesome :) Would you mind sending a pull request with the benchmark as well, this way other people working on optimizations can have some kind of reference to play with |
| @@ -1068,8 +1068,32 @@ var computeLayout = (function() { | |||
| child.nextAbsoluteChild = null; | |||
| } | |||
| } | |||
|
|
|||
| function layoutNode(node, parentMaxWidth, parentDirection) { | |||
| if (!node.lastLayout || | |||
There was a problem hiding this comment.
can you support node.isDirty as well
|
Actually... the benchmark results above are not really valid since it used the same tree every run, so everything was cached (duh). Still way better though if you're just modifying a child rather than the whole tree. I'll update it. |
|
Fixed nits and added node.isDirty, and node.shouldUpdate support. Also has to reset child layouts for any invalidated nodes before recomputing or the results will be wrong. |
| child.layout.height = undefined; | ||
| child.layout.top = 0; | ||
| child.layout.left = 0; | ||
| child.layout.bottom = 0; |
There was a problem hiding this comment.
hmm, what's going on with those two? We shouldn't compute those. They can be obtained from the other 4
|
Updated. |
| node.lastLayout.parentMaxWidth = parentMaxWidth; | ||
| node.lastLayout.direction = direction; | ||
|
|
||
| // Reset child layouts |
There was a problem hiding this comment.
You shouldn't have to reset children layout, we're not doing that in the C version. Can you figure out what's going on here?
There was a problem hiding this comment.
Looks like it's done in react-native instead of in this library. https://github.com/facebook/react-native/blob/e665acca4b8839424936b38b686daf2c0f9f7f74/React/Views/RCTShadowView.m#L161-L164
In the Java version it's done here though: https://github.com/facebook/css-layout/blob/cefd6ccb967ffccc877ee7bd80500c69c3a0292b/src/java/src/com/facebook/csslayout/LayoutEngine.java#L218-L220
There was a problem hiding this comment.
Hmm, looks like we either need to update the C version or the Java one. Sucks that we have 2 different ways of reseting the children :x
I think that the C version is more "correct" as we're reseting the nodes that have been dirtied, not some random children.
There was a problem hiding this comment.
Isn't it expected that all children of a node not have layout properties defined prior to processing that node? layoutNodeImpl seems to assume so. Without resetting, some isUndefined checks might fail where they shouldn't, and the position of a node is added to it's position from the previous layout pass. I think all children have to be reset. The C version just does this after a layout pass instead of before the next one.
There was a problem hiding this comment.
You are right, it doesn't matter much indeed if we do it before the beginning or after the end. It's probably better to do it inside of the layout algorithm this way the callsite doesn't have to care and possibly introduce bugs.
Implement caching in the JS version
This implements caching for the JS version, as suggested by @vjeux in #137. Here's the results of the same benchmark I used before, compared to the current master (after my previous optimizations):