Stop cloning options for a massive performance boost

[mstade]

This seemingly innocent operation is apparently responsible for
some serious performance bottlenecks. In testing, I've seen that
performance degrades by nearly an order of magnitute per file
imported. By not cloning options, this bottleneck goes away,
making this plugin significantly faster.

On a project with 9 @import rules running this plugin took on
average 7 seconds; by removing this clone operation it now takes
on average 100ms, with roughly 90% of that time spent in file I/O.

[mstade]

During this investigation, I spent some time looking at whether async file I/O would provide any benefits. Hypothetically, those 90ms spent in I/O could be turned to 10ms but running all reads in parallel; however in practice I was unable to achieve any such gains. It may be that my experiment still ran reads in sequence, even though it was using async reads – I might have just misunderstood the structure of the code. Regardless, this change seems to be very low hanging fruit for significantly improving performance.

Presumably, caching reads as per #47 would also provide gains but not in my case, because no file is depended on more than once.

[mstade]

I'll see about fixing that broken test.

[mstade]

Ah, I think I understand better why the cloning is there now in the first place – it's because options get mutated later. This obviously breaks the test. I have a potential solution to this which is significantly faster than cloning, however. Will push when I verify the tests pass.

[gabrielmontagne]

I'd say 👍

[TrySound]

@mstade I'd like to suggest for this case object-assign or extend-shallow

[MoOx]

Object-assign is already included btw

[mstade]

Sure – potato/tomato as far as I'm concerned since there's no practical difference between Object.create and Object.assign in this case, they both get the job done. The perf issue is what matters.

[mstade]

Heya! Any interest in getting this merged? I'm currently depending on my fork, and would love not to. If there's anything else missing from this PR before it can be merged, please let me know.

[MoOx]

I will take a deeper look this evening or tomorrow morning. I want to double check why I commited/merged something with clone in the first place.

[mstade]

Right on – much obliged!

[mstade]

FWIW, my cursory investigation suggest the cloning happens because the options get mutated down the line. If they aren't cloned, then subsequent imports may fail – this is why the test broke in my initial commit, so kudos for coverage!

Does this help?

[MoOx]

The clone was a mistake by me.

[MoOx]

Released as 7.1.1

[TrySound]

@MoOx Seems like not that PR

[MoOx]

Oh my bad lol. Releasing 7.1.2 sorry.

[TrySound]

@mstade You should use ponyfill. There isn't Object.assign on node 0.12

[mstade]

With all due respect @TrySound, you're just bike shedding. There was nothing wrong with using Object.create in the first place – it got the job done. Object.assign is semantically more appropriate, but there is no practical difference in this case. Node 0.12 compatibility wasn't part of the build at that point, so I couldn't really have known that was a requirement. Regardless, Object.create – again no practical difference – would've been a perfectly good stand-in for Object.assign and provide 0.12 compatibility without the need for yet another dependency with unknown performance characteristics.

I'm sorry to come across as rude, but there it is.

Thank you @MoOx for merging.

[TrySound]

@mstade Ah, yeah. exactly. Sorry. But still assign was already required.

Object-assign is already included btw

[TrySound]

@mstade The difference is here Object/assign

[mstade]

First, my apologies for the tone in my previous comment. Second, I do know the difference (but thank you for the link anyway!) and what I'm saying is that in this context the difference is irrelevant. The way I used Object.create only made properties of the original options object inherited, and any mutations to the copy would at worst shadow the original. As well I didn't find any reliance on not copying non-enumerable properties, so that isn't an issue either. That's why I'm saying there's only a (minor) semantic difference, and so relying on third party code is unnecessary (and potentially costly) when built-in options are available.

Regardless, I'm happy and thankful that this fix was merged; and after testing I can also happily report that using object-assign instead of a built-in option doesn't seem to adversely affect performance.