Performance Improvement in three JavaScript Source Codes

[yongbae]

By performance profiling in Internet Explorer 10, Firefox 17 and Chrome 25, I found performance improvement in wp-admin/js/post.js, wp-includes/js/media-views.js and wp-includes/js/utils.js. Function clean() at line number 17 in post.js, function set() at line number 98 in util.js and function visibility() at line number 2767 in media-views.js can be improved with same functionality.

I measured inclusive execution times of original code and improved code. The image is the result. The machine I used to measure execution times has Intel Core i5 1.6GHz CPU and 4GB RAM.

I also attached a patch file.

[yongbae]

Graph of inclusive execution time

[scribu]

So, all 3 changes amount to less than 1ms in speed, or am I reading the graph wrong?

[yongbae]

You are right.

[scribu]

Rather than doing micro-optimizations like this with little to no benefit, maybe your time would be better spent fixing all the jQuery deprecation warnings in #22975. Just saying.

[markoheijnen]

If everything still works as before I don't see any reason why not to commit this. The patch also cleans out the code a bit.

[scribu]

If everything still works as before I don't see any reason why not to commit this.

So, how about we start randomly refactoring code, just for the hell of it? As long as it doesn't break, it's all good, right?

[markoheijnen]

We optimize/refactor code pretty often. Like this commit [23410]. I don't say we should pick random code and just change it. That is bad and I can see that. This patch makes the code a little bit more readable too. But that just my opinion. If it gets closed as worksforme I'm fine with that too.

[scribu]

We rarely refactor purely for aesthetic reasons, though. And, as the graph shows, there simply isn't much performance gain.

[GaryJ]

But, it *is* a performance gain if the graph holds true, the work has already been done, and it's only a dozen or so lines to review for when a committer has got those files at the forefront of their mind. Even my cursory 30-second glance suggest the changes look sensible enough to warrant consideration.

Let's not be dismissive of any efforts to contribute, however small or off-topic you think it might be.

[adamsilverstein]

Hey @yongbae!

A belated thanks for your effort and patch. Curious what motivated you to analyze and then improve the performance of these JS files? We could really use your help optimizing the JS running the customizer if you are up for more :)

The original patch no longer applies, at all.

I reviewed the optimizations in the patch.

The changes to the tag regex, now in tags-box.js are minor, making some selected items for replacement optional. I don't think its worth changing the regex and risk some unexpected consequences, so I am dropping this change.

The optimization in media-views.js makes sense, it avoids setting up views and hide when they aren't getting used. Keeping that.

The optimization in util.js makes sense; don't set up date unless using, and cache the expensive parseInt call. Nice.

Refreshed patch incoming.

[adamsilverstein]

23473.diff​:

Refresh against trunk.

[wonderboymusic]

I can tell by looking at the patch that this is incomplete - media-views.js is a generated file, yet there's no source changes that would affect it. Also, this wouldn't pass jshint

[adamsilverstein]

Replying to wonderboymusic:

I can tell by looking at the patch that this is incomplete - media-views.js is a generated file, yet there's no source changes that would affect it. Also, this wouldn't pass jshint

Yea, I agree - this won't pass jshint, and the very thing jshint will deny is the thing thats buying us tiny marginal gains here. Plus your point that that media js is built makes all this moot. Well, it was fun while it lasted: felt like a class assignment :)

Closing as wontfix.