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

Handle shared attributes object in hyperscript #1942

Merged
merged 2 commits into from Aug 27, 2017

Conversation

Projects
None yet
3 participants
@s-ilya
Copy link
Contributor

s-ilya commented Aug 21, 2017

Possible fix for Shared attributes for different component results in concatenated class names #1941

Mithril modifies passed attributes object to store classname derived from selector. I suggest we make a copy so there will no unexpected side-effects.

btw, I'm using custom shallow copy instead of Object.assign because mithril seems to support IE9 and 10. Is it still the case?

@isiahmeadows
Copy link
Collaborator

isiahmeadows left a comment

See my comment for details.

But do note: we are limited to ES5 (excluding Promises) to support IE9/10 and many Android browsers, so yes, Object.assign is out of the window.

Also, if you could, please add a line to the changelog explaining this change (it's a bug fix, but it will affect performance optimization). It's not critical, but it'd make our lives easier.

attrs = Object.keys(attrs).reduce(function(newAttributes, key) {
newAttributes[key] = attrs[key]
return newAttributes
}, {})

This comment has been minimized.

@isiahmeadows

isiahmeadows Aug 23, 2017

Collaborator

Just use a (hasOwnProperty guarded) for ... in loop. Keep in mind this is in a somewhat performance-critical place, and we'd rather not be creating a closure here.

Also, this copying should only be done in execSelector, preferably only if state.attrs there is non-empty. That way, it is only done when it's absolutely necessary, not on every call.

Keep in mind, each render could literally create thousands of these nodes each time. It has to be fast in the common path.

This comment has been minimized.

@s-ilya

s-ilya Aug 23, 2017

Contributor

@isiahmeadows thanks for clarification! I will look into that in a day or two.

@s-ilya s-ilya force-pushed the s-ilya:hypertext-shared-attrs branch from c041eac to 9e6b175 Aug 23, 2017

@s-ilya s-ilya requested a review from tivac as a code owner Aug 23, 2017

@s-ilya

This comment has been minimized.

Copy link
Contributor

s-ilya commented Aug 23, 2017

@isiahmeadows I have updated PR:

  • copy attributes in execSelector if attrs and state.attrs aren't empty
  • update changelog
  • make tests a bit more explicit

Please note that I had to amend existing commit to include issue id there.

@@ -28,6 +28,18 @@ function execSelector(state, attrs, children) {
var hasAttrs = false, childList, text
var className = attrs.className || attrs.class

if (Object.keys(state.attrs).length && Object.keys(attrs).length) {

This comment has been minimized.

@isiahmeadows

isiahmeadows Aug 23, 2017

Collaborator

Note to self for later (if nobody beats me to it): introduce a more optimized helper in this file to check if an object has any properties.

function isEmpty(object) {
    for (var key of object) if (hasOwn.call(object, key)) return true
    return false
}

(Nothing mandatory on your part @s-ilya)

@tivac

This comment has been minimized.

Copy link
Member

tivac commented Aug 24, 2017

Would you npm run perf about 5x both before and after this change & post the results? I'm worried that this is going to add substantial runtime cost.

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Aug 25, 2017

@tivac I'll point out that part of the perf issue you'll catch is the Object.keys(object).length for detecting non-empty objects, something I've noted as a self-addressed TODO.

So if you implement that before benchmarking, you'll probably glean a better understanding of the end performance impact.

@tivac

This comment has been minimized.

Copy link
Member

tivac commented Aug 25, 2017

That's fine, I just want to understand the impact given that even when this is rewritten it looks like it'll require creating a brand-new attrs object and copying all the properties to it on every single redraw.

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Aug 25, 2017

@tivac Also, I did have @s-ilya move the copying logic to where it would have far less of a performance hit in practice. (Note: we're already copying the attrs object from parsed selectors to begin with. The difference is that it's not allocated then, but small object allocation is surprisingly cheap.)

@s-ilya

This comment has been minimized.

Copy link
Contributor

s-ilya commented Aug 25, 2017

@tivac @isiahmeadows please see results below: without changes, with changes and with changes + isEmpty function.

Without changes

medle@DESKTOP-A0U7GBR MINGW64 ~/mithril.js (next)
$ for i in {1..5}; do npm run perf; done

mithril@1.1.3 perf C:\Users\medle\mithril.js
node performance/test-perf.js

rerender without changes x 2,254,951 ops/sec ±2.74% (89 runs sampled)
construct large VDOM tree x 3,621 ops/sec ±2.68% (85 runs sampled)
mutate styles/properties x 22,014 ops/sec ±4.82% (72 runs sampled)
repeated trees (recycling) x 2,074 ops/sec ±4.59% (75 runs sampled)
repeated trees (no recycling) x 2,096 ops/sec ±5.32% (73 runs sampled)
Completed perf tests in 31581ms

mithril@1.1.3 perf C:\Users\medle\mithril.js
node performance/test-perf.js

rerender without changes x 2,222,602 ops/sec ±2.16% (87 runs sampled)
construct large VDOM tree x 3,601 ops/sec ±2.95% (84 runs sampled)
mutate styles/properties x 22,300 ops/sec ±3.88% (71 runs sampled)
repeated trees (recycling) x 2,093 ops/sec ±4.84% (73 runs sampled)
repeated trees (no recycling) x 2,091 ops/sec ±5.10% (62 runs sampled)
Completed perf tests in 31597ms

mithril@1.1.3 perf C:\Users\medle\mithril.js
node performance/test-perf.js

rerender without changes x 2,170,630 ops/sec ±2.96% (84 runs sampled)
construct large VDOM tree x 3,388 ops/sec ±2.80% (82 runs sampled)
mutate styles/properties x 21,095 ops/sec ±4.83% (69 runs sampled)
repeated trees (recycling) x 1,944 ops/sec ±4.42% (70 runs sampled)
repeated trees (no recycling) x 1,936 ops/sec ±5.98% (60 runs sampled)
Completed perf tests in 31364ms

mithril@1.1.3 perf C:\Users\medle\mithril.js
node performance/test-perf.js

rerender without changes x 1,887,043 ops/sec ±1.97% (83 runs sampled)
construct large VDOM tree x 2,999 ops/sec ±3.36% (78 runs sampled)
mutate styles/properties x 18,021 ops/sec ±3.97% (71 runs sampled)
repeated trees (recycling) x 1,779 ops/sec ±4.50% (77 runs sampled)
repeated trees (no recycling) x 1,752 ops/sec ±5.14% (66 runs sampled)
Completed perf tests in 31558ms

mithril@1.1.3 perf C:\Users\medle\mithril.js
node performance/test-perf.js

rerender without changes x 1,845,580 ops/sec ±4.47% (78 runs sampled)
construct large VDOM tree x 3,069 ops/sec ±2.92% (73 runs sampled)
mutate styles/properties x 16,823 ops/sec ±5.32% (68 runs sampled)
repeated trees (recycling) x 1,696 ops/sec ±5.13% (68 runs sampled)
repeated trees (no recycling) x 1,687 ops/sec ±4.03% (69 runs sampled)
Completed perf tests in 31729ms

With changes from this PR

medle@DESKTOP-A0U7GBR MINGW64 ~/mithril.js (hypertext-shared-attrs)
$ for i in {1..5}; do npm run perf; done

mithril@1.1.3 perf C:\Users\medle\mithril.js
node performance/test-perf.js

rerender without changes x 2,183,097 ops/sec ±2.31% (89 runs sampled)
construct large VDOM tree x 3,669 ops/sec ±1.76% (89 runs sampled)
mutate styles/properties x 22,653 ops/sec ±3.18% (76 runs sampled)
repeated trees (recycling) x 2,159 ops/sec ±4.01% (79 runs sampled)
repeated trees (no recycling) x 2,178 ops/sec ±4.82% (70 runs sampled)
Completed perf tests in 31553ms

mithril@1.1.3 perf C:\Users\medle\mithril.js
node performance/test-perf.js

rerender without changes x 2,307,716 ops/sec ±2.03% (91 runs sampled)
construct large VDOM tree x 3,824 ops/sec ±1.84% (91 runs sampled)
mutate styles/properties x 22,788 ops/sec ±3.15% (76 runs sampled)
repeated trees (recycling) x 2,217 ops/sec ±3.42% (77 runs sampled)
repeated trees (no recycling) x 2,231 ops/sec ±4.32% (77 runs sampled)
Completed perf tests in 32290ms

mithril@1.1.3 perf C:\Users\medle\mithril.js
node performance/test-perf.js

rerender without changes x 2,317,578 ops/sec ±1.71% (87 runs sampled)
construct large VDOM tree x 3,605 ops/sec ±2.76% (82 runs sampled)
mutate styles/properties x 23,023 ops/sec ±3.43% (73 runs sampled)
repeated trees (recycling) x 2,161 ops/sec ±5.13% (76 runs sampled)
repeated trees (no recycling) x 2,101 ops/sec ±4.25% (75 runs sampled)
Completed perf tests in 30818ms

mithril@1.1.3 perf C:\Users\medle\mithril.js
node performance/test-perf.js

rerender without changes x 2,179,205 ops/sec ±3.39% (87 runs sampled)
construct large VDOM tree x 3,543 ops/sec ±2.69% (83 runs sampled)
mutate styles/properties x 23,261 ops/sec ±3.08% (74 runs sampled)
repeated trees (recycling) x 2,113 ops/sec ±5.31% (76 runs sampled)
repeated trees (no recycling) x 2,160 ops/sec ±5.07% (67 runs sampled)
Completed perf tests in 31685ms

mithril@1.1.3 perf C:\Users\medle\mithril.js
node performance/test-perf.js

rerender without changes x 2,242,537 ops/sec ±3.22% (87 runs sampled)
construct large VDOM tree x 3,712 ops/sec ±1.96% (88 runs sampled)
mutate styles/properties x 23,091 ops/sec ±4.26% (77 runs sampled)
repeated trees (recycling) x 2,166 ops/sec ±4.85% (74 runs sampled)
repeated trees (no recycling) x 2,160 ops/sec ±4.54% (72 runs sampled)
Completed perf tests in 31725ms

With changes from this PR + isEmpty function

medle@DESKTOP-A0U7GBR MINGW64 ~/mithril.js (hypertext-shared-attrs)
$ for i in {1..5}; do npm run perf; done

mithril@1.1.3 perf C:\Users\medle\mithril.js
node performance/test-perf.js

rerender without changes x 2,352,263 ops/sec ±1.82% (90 runs sampled)
construct large VDOM tree x 3,797 ops/sec ±1.84% (92 runs sampled)
mutate styles/properties x 19,113 ops/sec ±5.43% (70 runs sampled)
repeated trees (recycling) x 2,138 ops/sec ±4.84% (70 runs sampled)
repeated trees (no recycling) x 2,193 ops/sec ±3.70% (78 runs sampled)
Completed perf tests in 31987ms

mithril@1.1.3 perf C:\Users\medle\mithril.js
node performance/test-perf.js

rerender without changes x 2,355,014 ops/sec ±1.85% (90 runs sampled)
construct large VDOM tree x 3,798 ops/sec ±2.19% (87 runs sampled)
mutate styles/properties x 23,048 ops/sec ±3.45% (76 runs sampled)
repeated trees (recycling) x 2,084 ops/sec ±5.52% (73 runs sampled)
repeated trees (no recycling) x 2,159 ops/sec ±4.83% (73 runs sampled)
Completed perf tests in 31754ms

mithril@1.1.3 perf C:\Users\medle\mithril.js
node performance/test-perf.js

rerender without changes x 2,272,959 ops/sec ±2.98% (92 runs sampled)
construct large VDOM tree x 3,738 ops/sec ±2.35% (88 runs sampled)
mutate styles/properties x 22,867 ops/sec ±3.16% (77 runs sampled)
repeated trees (recycling) x 2,139 ops/sec ±4.79% (74 runs sampled)
repeated trees (no recycling) x 2,120 ops/sec ±4.69% (68 runs sampled)
Completed perf tests in 31700ms

mithril@1.1.3 perf C:\Users\medle\mithril.js
node performance/test-perf.js

rerender without changes x 2,282,442 ops/sec ±2.19% (90 runs sampled)
construct large VDOM tree x 3,784 ops/sec ±2.05% (87 runs sampled)
mutate styles/properties x 23,050 ops/sec ±3.76% (77 runs sampled)
repeated trees (recycling) x 2,191 ops/sec ±3.65% (78 runs sampled)
repeated trees (no recycling) x 2,155 ops/sec ±3.94% (75 runs sampled)
Completed perf tests in 31668ms

mithril@1.1.3 perf C:\Users\medle\mithril.js
node performance/test-perf.js

rerender without changes x 2,191,162 ops/sec ±3.73% (86 runs sampled)
construct large VDOM tree x 3,816 ops/sec ±1.75% (90 runs sampled)
mutate styles/properties x 23,086 ops/sec ±3.04% (78 runs sampled)
repeated trees (recycling) x 2,163 ops/sec ±3.87% (77 runs sampled)
repeated trees (no recycling) x 2,153 ops/sec ±5.00% (66 runs sampled)
Completed perf tests in 31876ms

@tivac

This comment has been minimized.

Copy link
Member

tivac commented Aug 25, 2017

@s-ilya awesome, thanks for the info. My perf concerns are quelled!

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Aug 26, 2017

@s-ilya Since you already tried it with the isEmpty pseudo-suggestion, mind tacking that onto this PR?

@s-ilya

This comment has been minimized.

Copy link
Contributor

s-ilya commented Aug 26, 2017

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Aug 27, 2017

Full merge ahead! 😄

@isiahmeadows isiahmeadows merged commit c9629ff into MithrilJS:next Aug 27, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment