WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 4 months ago

#23473 closed enhancement (wontfix)

Performance Improvement in three JavaScript Source Codes

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

Description

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 4 months ago.

Download all attachments as: .zip

Change History (18)

@yongbae
3 years ago

Graph of inclusive execution time

@yongbae
3 years ago

#1 @SergeyBiryukov
3 years ago

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

#2 @scribu
3 years ago

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

Last edited 3 years ago by scribu (previous) (diff)

#3 @yongbae
3 years ago

You are right.

#4 @scribu
3 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.

#5 @markoheijnen
3 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.

#6 @scribu
3 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?

#7 @markoheijnen
3 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.

#8 follow-up: @scribu
3 years ago

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

#9 in reply to: ↑ 8 @GaryJ
3 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.

#10 @nacin
2 years ago

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

#11 @chriscct7
4 months ago

  • Keywords needs-refresh added

#12 @adamsilverstein
4 months 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.

#13 @adamsilverstein
4 months ago

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

23473.diff:

Refresh against trunk.

#14 follow-up: @wonderboymusic
4 months ago

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

#15 in reply to: ↑ 14 @adamsilverstein
4 months ago

  • Milestone 4.4 deleted
  • Resolution set to wontfix
  • Status changed from assigned to closed

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.

Note: See TracTickets for help on using tickets.