Make WordPress Core

Opened 3 years ago

Last modified 43 hours ago

#23473 assigned enhancement

Performance Improvement in three JavaScript Source Codes

Reported by: yongbae Owned by: adamsilverstein
Milestone: 4.4 Priority: normal
Severity: normal Version: 3.5
Component: Administration Keywords: has-patch dev-feedback
Focuses: javascript, performance Cc:


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.

Attachments (3)

graph.png (28.8 KB) - added by yongbae 3 years ago.
Graph of inclusive execution time
performance.patch (2.3 KB) - added by yongbae 3 years ago.
23473.diff (1.6 KB) - added by adamsilverstein 43 hours ago.

Download all attachments as: .zip

Change History (16)

@yongbae3 years ago

Graph of inclusive execution time

@yongbae3 years ago

comment:1 @SergeyBiryukov3 years ago

  • Component changed from General to Performance
  • Keywords has-patch added

comment:2 @scribu3 years ago

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

Version 0, edited 3 years ago by scribu (next)

comment:3 @yongbae3 years ago

You are right.

comment:4 @scribu3 years ago

  • Keywords close added

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.

comment:5 @markoheijnen3 years ago

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.

comment:6 @scribu3 years ago

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?

comment:7 @markoheijnen3 years ago

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.

comment:8 follow-up: @scribu3 years ago

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

comment:9 in reply to: ↑ 8 @GaryJ3 years ago

  • Keywords close removed

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.

comment:10 @nacin21 months ago

  • Component changed from Performance to Administration
  • Focuses javascript performance added

comment:11 @chriscct72 days ago

  • Keywords needs-refresh added

comment:12 @adamsilverstein43 hours ago

  • Owner set to adamsilverstein
  • Status changed from new to assigned

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.

@adamsilverstein43 hours ago

comment:13 @adamsilverstein43 hours ago

  • Keywords dev-feedback added; needs-refresh removed
  • Milestone changed from Awaiting Review to 4.4


Refresh against trunk.

Note: See TracTickets for help on using tickets.