Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

perf(jqLite): implement and use the empty method in place of html(‘’) #4457

Closed
wants to merge 1 commit into from

Conversation

mgol
Copy link
Member

@mgol mgol commented Oct 16, 2013

jQuery's elem.html('') is way slower than elem.empty(). As clearing
element contents happens quite often in certain scenarios, switching
to using .empty() provides a significant performance boost when using
Angular with jQuery.

An example benchmark: http://jsperf.com/html-vs-empty shows extreme performance gains.

It would be great to have it for 1.2.0, especially that it doesn't introduce any breaking changes.

On our current app on a very heavy page the load time was lowered from 2.6s to 2s after applying this patch which gives >20% performance boost.

@mary-poppins
Copy link

Thanks for the PR!

  • Contributor signed CLA now or in the past
    • If you just signed, leave a comment here with your real name
  • PR's commit messages follow the commit message format

If you need to make changes to your pull request, you can update the commit with git commit --amend.
Then, update the pull request with git push -f.

Thanks again for your help!

@mgol
Copy link
Member Author

mgol commented Oct 16, 2013

@mary-poppins I'm just ignoring you from now on, you liar! ;)

@bfricka
Copy link

bfricka commented Oct 20, 2013

@mzgol, I'm curious as to why you chose to use element.innerHTML = ''; instead of the significantly faster:

while (element.firstChild) {
  element.removeChild(element.firstChild);
}

Source

@mgol
Copy link
Member Author

mgol commented Oct 20, 2013

@brian-frichette Good catch! Corrected.

@bfricka
Copy link

bfricka commented Oct 21, 2013

👍

@mgol
Copy link
Member Author

mgol commented Oct 28, 2013

@IgorMinar any chance it'll get into 1.2.0? Or is it too late for that?

@btford
Copy link
Contributor

btford commented Oct 28, 2013

We are not adding new features to 1.2.0 unless it is the best way to fix some critical issue.

@mgol
Copy link
Member Author

mgol commented Oct 28, 2013

Ok, got it. I guess sth like that can wait for 1.2.1.

@mgol
Copy link
Member Author

mgol commented Nov 23, 2013

@btford Any chance of getting it merge soon-ish or not really? This provides considerable performance gains for our app but it's a pain to rebase it as it touches an awful lot places in the code base and there are conflicts very often...

@sicarrots
Copy link

@btford any chance of getting this merged soon? This patch really improve overall performance of angular.js-base apps.

@jrencz
Copy link

jrencz commented Dec 9, 2013

I'm looking forward to this change and hope It'll be merged soon

…‘’)`

jQuery's elem.html('') is way slower than elem.empty(). As clearing
element contents happens quite often in certain scenarios, switching
to using .empty() provides a significant performance boost when using
Angular with jQuery.
@ghost ghost assigned IgorMinar Dec 13, 2013
@@ -54,7 +54,7 @@ if (window.jQuery) {
});

it('should fire on html(\'\')', function() {
doc.html('');
doc.empty();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this change doesn't make sense. search&replace error?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird, I was cautious about such a change in another place but I had to miss it here.

@IgorMinar IgorMinar closed this in 3410f65 Dec 13, 2013
@IgorMinar
Copy link
Contributor

nice PR. thanks!

sorry for not merging it sooner. it didn't get tagged properly during initial triage and I haven't noticed it until now.

@mgol
Copy link
Member Author

mgol commented Dec 13, 2013

Thanks for merging!

@mgol mgol deleted the empty branch December 13, 2013 10:47
jamesdaily pushed a commit to jamesdaily/angular.js that referenced this pull request Jan 27, 2014
…‘’)`

jQuery's elem.html('') is way slower than elem.empty(). As clearing
element contents happens quite often in certain scenarios, switching
to using .empty() provides a significant performance boost when using
Angular with jQuery.

Closes angular#4457
jamesdaily pushed a commit to jamesdaily/angular.js that referenced this pull request Jan 27, 2014
…‘’)`

jQuery's elem.html('') is way slower than elem.empty(). As clearing
element contents happens quite often in certain scenarios, switching
to using .empty() provides a significant performance boost when using
Angular with jQuery.

Closes angular#4457
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants