Bug Tracker

Modify

Ticket #10682 (closed bug: fixed)

Opened 2 years ago

Last modified 2 years ago

Creating DOM elements with $(' ') leaks memory and skips the fragment cache

Reported by: niek.sanders@… Owned by: rwaldron
Priority: blocker Milestone: 1.7.1
Component: core Version: 1.7
Keywords: Cc:
Blocking: Blocked by:

Description

Creating DOM elements with $() can cause memory usage to increase in a leak-like manner.

Full fiddles to reproduce this bug are given below. In brief... every time this routine is called, jQuery sucks up more memory:

  fred$.empty();
  for ( var j = 0; j < 10; j++ ) {
    $('<li>' + j + '</li>').appendTo( fred$ );
  }

I'm guessing the problem lies in the selector cache. If we run the exact same loop again, it's not finding the cached results. This code, in contrast, does not have memory-growth issues:

  fred$.empty();
  for ( var j = 0; j < 10; j++ ) {
    $('<li></li>').appendTo( fred$ );
  }

I've also found that using html() can work around the bug:

 fred$.empty();
  for ( var j = 0; j < 10; j++ ) {
    $('<li></li>').html(j).appendTo( fred$ );
  }

Here are two fiddles which illustrate the issue:

To reproduce, take heap snapshots in Chrome. Everytime the "Show Bug" button in clicked in the leaky version, the heap usage will increase by about 1.5 MB. The "good" version will have constant memory usage under the same test.

I'm using Linux and can reproduce the issue in both Chrome 15.0.874.106 and Firefox 7.0.1. For Chrome, I used the heap snapshots after each click. For Firefox, I used the system resource monitor to see the memory usage change.

Change History

comment:1 Changed 2 years ago by rwaldron

  • Status changed from new to closed
  • Resolution set to invalid

The memory consumption in this pattern will be the same with or without jQuery. Create a cached LI and clone it in each iteration. The second thing is to append new elements to a dummy fragment in the loop then move the to the real target after the loop ( this will reduce reflows)

comment:2 Changed 2 years ago by anonymous

So doing: 1) Create ten elements 2) Delete those ten elements 3) Go back to step 1

Resulting in constantly increasing memory usage is expected behavior? Yipes.

comment:3 Changed 2 years ago by dmethvin

Any sufficiently advanced cache algorithm is indistinguishable from a memory leak. :-) Processing HTML into DOM elements is expensive, which is why we cache HTML fragments and their corresponding DOM elements.

If you create ten *unique* html fragments smaller than 512 bytes then the fragment cache will have ten new entries. A line of code like $("<li />").attr("data-abc", n) creates only one entry in the cache even if executed repeatedly but $("<li data-abc='"+j+"' />") creates a cache entry for each unique value of j. Which is what you have verified.

So we have some options here. We can document the best practice of not creating lots of unique HTML strings, or we can have a way to shut off the fragment cache so your unique HTML strings won't be cached but your code will run slower. Given that you're generating a LOT of these, I suspect the page is already pretty large and slow. So it seems like documenting the behavior of the fragment cache and advising the style in your second example would be best. Do you agree?

As a hacky and unsupported workaround you could try setting jQuery.fragments = {} since that will empty the cache, but that might cause worse performance issues than the memory problems it solves.

comment:4 Changed 2 years ago by niek.sanders@…

Your caching explanation makes perfect sense.

There is just one small thing that troubles me.... if you look at the js Fiddle, there are only ten unique strings being fed to jQuery. It should be hitting the cache after each iteration of the loop yet it seems to be creating new cache entries.

comment:5 Changed 2 years ago by dmethvin

  • Status changed from closed to reopened
  • Component changed from unfiled to core
  • Summary changed from Creating DOM elements with $(' ') leaks memory. to Creating DOM elements with $(' ') leaks memory and skips the fragment cache
  • Priority changed from undecided to blocker
  • Milestone changed from None to 1.7.1
  • Resolution invalid deleted

Well just to be sure I put some debugging in the fiddle to dump the fragment cache ... and it's empty! So we have a different bug here, it looks like the condition to check for HTML5 validity in IE is borking things.

 http://jsfiddle.net/dmethvin/MQZtY/7/

So we'll have to get that fixed before we can take it further. I'm reopening the ticket.

comment:6 Changed 2 years ago by anonymous

I ran your fiddle in both Chrome and Firefox on Linux. Empty fragment cache on both of those too.

"jQuery.fragments has 0 entries with 0 key bytes"

(both before and after the loop)

comment:7 Changed 2 years ago by timmywil

  • Status changed from reopened to open

comment:8 Changed 2 years ago by rwaldron

I'm willing to bet that the rnoshimcache condition can be removed now that we're fake cloning elements (in my pending patch)

comment:9 Changed 2 years ago by rwaldron

My previous suggestion is incorrect, looking for a different fix and writing expected jQuery.fragments tests.

comment:10 Changed 2 years ago by rwaldron

  • Owner set to rwaldron
  • Status changed from open to assigned

comment:11 Changed 2 years ago by timmywil

  • Status changed from assigned to closed
  • Resolution set to fixed

Remove test of the invalid object for IE9's sake; Rewrite the appropriate support test for html5 clone caching. Fixes #10682

Changeset: 41b31d7386cf00e714d703db773ffa73b6bd8b24

Please follow the  bug reporting guidlines and use  jsFiddle when providing test cases and demonstrations instead of pasting the code in the ticket.

View

Add a comment

Modify Ticket

Action
as closed
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.