Update: Big Perf improvement for IE (fixes #199)

[gyandeeps]

Ref: #199 (comment)

Using latest of mithril and reactjs (15.0.1).

Before

IE-10

Table Rows	Mithril	ReactJs
100	37-41	38-44
500	162-165	147-166
1000	335-355	285-305

Chrome

Table Rows	Mithril	ReactJs
100	27-35	57-62
500	85-91	176-185
1000	125-175	295-315

After

IE-10

Table Rows	Mithril	ReactJs
100	21 - 31	38-44
500	65 - 75	147-166
1000	125 - 135	285-305

Chrome

Table Rows	Mithril	ReactJs
100	27-35	57-62
500	85-91	176-185
1000	125-175	295-315

Perf test findings: https://github.com/spicyj/innerhtml-vs-createelement-vs-clonenode

Guys, I am new to this library so do let me know if more changes are required. Thanks

[tivac]

Interesting!

What code are you benchmarking here? I'd like to see that as well.

[gyandeeps]

@tivac The code is all internal so I cant put it here. But I have created a low level test for you where you can see the time on console.

Use this html file and make it point towards the mithril file: https://gist.github.com/gyandeeps/54b00e50587ef42478db97b91a88c632

You can run this against my fork and mithril repo to see the difference on IE.

For this test

Before: ~270ms
After: ~75ms

[dead-claudia]

Does this even work correctly? I'm skeptical whether it does.

Also, batching updates is a thing: it pays off a little on other browsers as well.

[gyandeeps]

Why do you think, this will not work?
have u tried my example from above?

[dead-claudia]

This line doesn't mirror the other implementation of parentElement.insertBefore(node, parentElement.childNodes[index] || null).

That's why I'm skeptical. I'm running Linux, so I can't exactly test this readily, but that part is sending off alarms in my head.

[gyandeeps]

I am still new to this library and have started using this in our big framework at work.
This is where i am looking for some feedback on how things work inside mihtril. The examples I have tried it on works great.

Also its not a small perf difference, its big on IE. https://github.com/spicyj/innerhtml-vs-createelement-vs-clonenode

UPDATE: I think its easy to implement that behavior since i just have to make sure the array gets created correctly.

Can you help me setup a scenario where i can see the issue happening so that i can work on it?

[lhorie]

Hey @gyandeeps

Sorry for the delayed response, there was a regression in the old test suite that I needed to sort out before I could look into this.

I tried to merge this and run it against the test suite with the applyIEPerf flag forcefully set to true, but it breaks about 39 tests in the mocha test suite (and 36 in the old test suite).

I'd recommend that you try running the test suite with lines 221-229 changed to var applyIEPerf = true to ensure your change doesn't introduce regressions.

[gyandeeps]

@lhorie Thanks for the feedback. I think as @isiahmeadows pointed out the issue above. I will get that fixed.
What are your thoughts on approach in general?

[lhorie]

I'm curious how speed would be affected in non-IE browsers if they ran the IE code, and I'm worried that this might break something like value or <option>s in <select>s where order of operations matters (even though in theory it shouldn't).

[gyandeeps]

Based on my tests/observation, this technique has close to 0 impact on non-IE browsers.

[dead-claudia]

@lhorie I suspect it could pay off on other browsers due to batching, but I'm not sure.

I would rather the _children property be a separate variable, though. I think it'd work nicer IMHO, since it's not mutating the object.

[dead-claudia]

@gyandeeps I think @lhorie meant with var applyIEPerf = true, and I'm also curious about that.

[gyandeeps]

Yes, with that only. But i can test some more and get back to u guys.

[lhorie]

If it has no impact performance wise, then I'd also recommend removing the conditional altogether. It's a lot easier to reason about code when it doesn't branch to different subroutines in different browsers

[dead-claudia]

@lhorie Agreed, and also if that perf benefit carries over. 😄

[gyandeeps]

Thanks guys, will

• Remove the condition
• Get all the test passing

[dead-claudia]

@lhorie Would you agree with me that this is better as a separate variable, not an added property?

[lhorie]

imho, if the goal is to change the order of appends, it may make more sense to change it directly in lines 883-914 than to hack the internals of insertNode

[dead-claudia]

You do have a good point. I've been busy lately working on other things unrelated to Mithril, so my mind isn't quite as fresh with dealing with Mithril's internals as it was before.

[gyandeeps]

@lhorie @isiahmeadows I have made the changes as you recommended. Its now works for all cases and also this is a clean approach rather than hacking into insertNode.

On IE

Before: ~270ms
After: ~155ms

Chrome does have some improvement but almost negligible.

[tivac]

That change looks way more understandable now, nice. 👌

[dead-claudia]

@gyandeeps

I like this much better. Does it also test against the old suite? I know that doesn't run through CI (even though it should IMHO, PRs accepted), but just to make sure.

[gyandeeps]

No Sir. Since i am new here, i rely on npm test to do all the checks.

[lhorie]

Tests look good, merging. Thanks @gyandeeps

[gyandeeps]

thanks @lhorie