New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

jQuery.data first access performance very bad #1728

Closed
mgol opened this Issue Oct 20, 2014 · 19 comments

Comments

Projects
None yet
5 participants
@mgol
Member

mgol commented Oct 20, 2014

Originally reported by daliuskal@… at: http://bugs.jquery.com/ticket/14839

 http://jsfiddle.net/6DLSw/12/ 2s+ first time, ~50ms later on. The first run is extremely slow.

 http://jsfiddle.net/6DLSw/13/ ~50ms when jQuery.data(el) was already called (note that the key doesn't matter).

The bug seems to have started with version 2.0.2 or so, when the jQuery.data was rewritten.

Using 1.11.0 on first case takes ~190ms first time, other times it's ~60ms.

This is affecting jquery ui performance very badly, e.g.  http://jsfiddle.net/6DLSw/6/ - first red rectangle drop takes 2s+.

Issue reported for jQuery 2.1.0

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 20, 2014

Member

Comment author: rwaldron

The internal design of jQuery.data and jQuery.fn.data will be changing again in a forth coming release.

Member

mgol commented Oct 20, 2014

Comment author: rwaldron

The internal design of jQuery.data and jQuery.fn.data will be changing again in a forth coming release.

@mgol mgol added this to the 3.0.0 milestone Oct 20, 2014

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 20, 2014

Member

Comment author: dmethvin

See also #11570 and  https://github.com/jquery/jquery/pull/1428

Member

mgol commented Oct 20, 2014

Comment author: dmethvin

See also #11570 and  https://github.com/jquery/jquery/pull/1428

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 20, 2014

Member

Comment author: dmethvin

#15059 is a duplicate of this ticket.

Member

mgol commented Oct 20, 2014

Comment author: dmethvin

#15059 is a duplicate of this ticket.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 20, 2014

Member

Comment author: jbedard

Sorry for the dupe.

Is the internal design change the solution proposed for #11570? Or are there more plans? The pull request for #11570 is still using Object.defineProperties, so I assume the data creation will still be very slow...

Member

mgol commented Oct 20, 2014

Comment author: jbedard

Sorry for the dupe.

Is the internal design change the solution proposed for #11570? Or are there more plans? The pull request for #11570 is still using Object.defineProperties, so I assume the data creation will still be very slow...

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 20, 2014

Member

Comment author: dmethvin

Yes, the PR is mainly a proof of concept to see if we could attach data directly to DOM elements to avoid doing cleanup and reduce the chances of leaks. I suspect we'll need to abandon the nice encapsulation to get speed back.

Member

mgol commented Oct 20, 2014

Comment author: dmethvin

Yes, the PR is mainly a proof of concept to see if we could attach data directly to DOM elements to avoid doing cleanup and reduce the chances of leaks. I suspect we'll need to abandon the nice encapsulation to get speed back.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 20, 2014

Member

Comment author: rwaldron

I'd like to see benchmarks for patterns that reflect real pathological use cases—accessing jQuery data on 60k divs at once isn't realistic.

Not using Object.defineProperty means:

  • the expando property is visible on all objects with data, for elements this isn't really a big deal, but extant code wants to use var o = {}; $(o).data(...) which means that o will now have an writable, configurable and enumerable property for _both_ user data and jQuery internal data.
  • data properties appearing in for-in, the array returned by Object.keys and jQuery.each iterations
  • data properties appearing in JSON, which can be fixed by falling back to the old toJSON hack
  • ...but then you have 1 too many properties on every object, which fails tests. Of course this too can be patched, but then the object that exists as the value of this expando won't match what's returned by .data()
Member

mgol commented Oct 20, 2014

Comment author: rwaldron

I'd like to see benchmarks for patterns that reflect real pathological use cases—accessing jQuery data on 60k divs at once isn't realistic.

Not using Object.defineProperty means:

  • the expando property is visible on all objects with data, for elements this isn't really a big deal, but extant code wants to use var o = {}; $(o).data(...) which means that o will now have an writable, configurable and enumerable property for _both_ user data and jQuery internal data.
  • data properties appearing in for-in, the array returned by Object.keys and jQuery.each iterations
  • data properties appearing in JSON, which can be fixed by falling back to the old toJSON hack
  • ...but then you have 1 too many properties on every object, which fails tests. Of course this too can be patched, but then the object that exists as the value of this expando won't match what's returned by .data()
@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 20, 2014

Member

Comment author: rwaldron

data properties appearing in JSON, which can be fixed by falling back to the old toJSON hack

This also means breaking:

  • jQuery.hasData agrees no data exists even when an empty data obj exists
  • data() returns entire data object with expected properties
  • Retrieve data object from a wrapped JS object (#7524)
  • removeData does not clear the object
  • Make sure that the right number of properties came through.
  • Data appears as expected.
  • Data is empty.
  • Sanity Check
  • The data didn't change even though the data-\* attrs did

I'm confident all of these can be "fixed" but I'm concerned that the resulting code will be a maintenance nightmare.

Member

mgol commented Oct 20, 2014

Comment author: rwaldron

data properties appearing in JSON, which can be fixed by falling back to the old toJSON hack

This also means breaking:

  • jQuery.hasData agrees no data exists even when an empty data obj exists
  • data() returns entire data object with expected properties
  • Retrieve data object from a wrapped JS object (#7524)
  • removeData does not clear the object
  • Make sure that the right number of properties came through.
  • Data appears as expected.
  • Data is empty.
  • Sanity Check
  • The data didn't change even though the data-\* attrs did

I'm confident all of these can be "fixed" but I'm concerned that the resulting code will be a maintenance nightmare.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 20, 2014

Member

Comment author: jbedard

I'm not sure if the defineProperty/ies is the only or main cause of this, I thought it was ( http://jsperf.com/v1-v2-data), but after a quick test replacing it with a plain assignment my use case was still significantly slower then jQuery1 (but this was 1 quick test...). If the defineProp is a big factor could a simple assignment be used for elements and still use defineProp for other objects?

My use case is a table where every cell has data. With approx 10k cells (500 rows, 20 columns) jQuery2 loads about 66% slower (~2.5s vs ~1.5s). That extra 1s is mainly Data.key (~700ms) and GC (~300ms).

Here's an example (50 rows x 10 columns) that seems to reproduce it:  http://jsperf.com/jq2-data/5

From profiling the 2 cases it looks like this jsperf reproduces almost the exact same issue I'm having. With jQuery1 the forced relayout is most expensive (~25%), the clone/append/data/GC are all fairly small (2-5% each). With jQuery2 Data.key (~30%) and GC (~25%) skyrocket making the relayout only ~10%.

It looks like this might only really effect chrome though? That surprised me...

Member

mgol commented Oct 20, 2014

Comment author: jbedard

I'm not sure if the defineProperty/ies is the only or main cause of this, I thought it was ( http://jsperf.com/v1-v2-data), but after a quick test replacing it with a plain assignment my use case was still significantly slower then jQuery1 (but this was 1 quick test...). If the defineProp is a big factor could a simple assignment be used for elements and still use defineProp for other objects?

My use case is a table where every cell has data. With approx 10k cells (500 rows, 20 columns) jQuery2 loads about 66% slower (~2.5s vs ~1.5s). That extra 1s is mainly Data.key (~700ms) and GC (~300ms).

Here's an example (50 rows x 10 columns) that seems to reproduce it:  http://jsperf.com/jq2-data/5

From profiling the 2 cases it looks like this jsperf reproduces almost the exact same issue I'm having. With jQuery1 the forced relayout is most expensive (~25%), the clone/append/data/GC are all fairly small (2-5% each). With jQuery2 Data.key (~30%) and GC (~25%) skyrocket making the relayout only ~10%.

It looks like this might only really effect chrome though? That surprised me...

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 20, 2014

Member

Comment author: jbedard

It looks like Chrome is acting up due to the long data expando property name. I assume the longer name is treated differently or causes the element properties to be stored differently?

This seems to fix everything:  https://github.com/jbedard/jquery/commit/ed3a9887d16b098a7c784cc4920185d9e5c53e2a

That change makes  http://jsperf.com/jq2-data/5 run 3x faster in chrome. It makes  http://jsfiddle.net/6DLSw/12/ go from 3000+ to ~500 on the first run. It removes all the GC CPU usage. And it made a test I had locally (basically  http://jsperf.com/jq2-data/5 called 100x) go from a 470m heap to 27m.

Here's an example showing the setting of different sized property names on elements:  http://jsperf.com/long-name-on-dom In this example FF/IE actually improve more then Chrome so I don't understand why the original bug seems to mainly effect Chrome and not the others. Maybe only Chrome also has the memory issues?

Also note that using plain assignment instead of defineProperty/ies (maybe only for elements?) still makes it another 5x faster. Only then does Data.key get reduced enough that it uses less time then the DOM manipulations (like v1) in the jsperf and jsfiddle examples...

Member

mgol commented Oct 20, 2014

Comment author: jbedard

It looks like Chrome is acting up due to the long data expando property name. I assume the longer name is treated differently or causes the element properties to be stored differently?

This seems to fix everything:  https://github.com/jbedard/jquery/commit/ed3a9887d16b098a7c784cc4920185d9e5c53e2a

That change makes  http://jsperf.com/jq2-data/5 run 3x faster in chrome. It makes  http://jsfiddle.net/6DLSw/12/ go from 3000+ to ~500 on the first run. It removes all the GC CPU usage. And it made a test I had locally (basically  http://jsperf.com/jq2-data/5 called 100x) go from a 470m heap to 27m.

Here's an example showing the setting of different sized property names on elements:  http://jsperf.com/long-name-on-dom In this example FF/IE actually improve more then Chrome so I don't understand why the original bug seems to mainly effect Chrome and not the others. Maybe only Chrome also has the memory issues?

Also note that using plain assignment instead of defineProperty/ies (maybe only for elements?) still makes it another 5x faster. Only then does Data.key get reduced enough that it uses less time then the DOM manipulations (like v1) in the jsperf and jsfiddle examples...

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 20, 2014

Member

Comment author: dmethvin

Awesome, @jbedard, thanks for drilling down on this! It seems like we should be able to use just a uid there, the critical thing is that we allow multiple jQueries to work on an element without conflict and jQuery.expando should ensure that.

@rwaldron, what do you think?

Member

mgol commented Oct 20, 2014

Comment author: dmethvin

Awesome, @jbedard, thanks for drilling down on this! It seems like we should be able to use just a uid there, the critical thing is that we allow multiple jQueries to work on an element without conflict and jQuery.expando should ensure that.

@rwaldron, what do you think?

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 20, 2014

Member

Comment author: jbedard

 https://code.google.com/p/chromium/issues/detail?id=378607 might be the cause of this, meaning it would be the '.' from Math.random() causing the issue not the length of the string.

Member

mgol commented Oct 20, 2014

Comment author: jbedard

 https://code.google.com/p/chromium/issues/detail?id=378607 might be the cause of this, meaning it would be the '.' from Math.random() causing the issue not the length of the string.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 20, 2014

Member

Comment author: dmethvin

That's interesting. At least the fix would be easy.

Member

mgol commented Oct 20, 2014

Comment author: dmethvin

That's interesting. At least the fix would be easy.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 20, 2014

Member

Comment author: jbedard

It looks like Sizzle also puts a property onto DOM elements containing a dash ("sizzle-timestamp")...

Any update on this? It would be great to at least remove the "-"s for now to avoid the main chrome issue...

Member

mgol commented Oct 20, 2014

Comment author: jbedard

It looks like Sizzle also puts a property onto DOM elements containing a dash ("sizzle-timestamp")...

Any update on this? It would be great to at least remove the "-"s for now to avoid the main chrome issue...

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 20, 2014

Member

Comment author: jbedard

The "delete elem[ internalKey ]" in $.cleanData seems to cause the same thing. When combined with  https://code.google.com/p/v8/issues/detail?id=2073#c26 this can lead to chrome crashing. In my case I'm deleting a giant grid with >300k nodes, and the delete line cause each node to increase in memory over 10x.

Member

mgol commented Oct 20, 2014

Comment author: jbedard

The "delete elem[ internalKey ]" in $.cleanData seems to cause the same thing. When combined with  https://code.google.com/p/v8/issues/detail?id=2073#c26 this can lead to chrome crashing. In my case I'm deleting a giant grid with >300k nodes, and the delete line cause each node to increase in memory over 10x.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 20, 2014

Member

Comment author: markelog

After landing  https://github.com/jquery/jquery/pull/1662 it seems we improved here, but

Also note that using plain assignment instead of defineProperty/ies (maybe only for elements?) still makes it another 5x faster

Sounds like using defineProperties is bad idea, as of now that is, should we get back to using plain assignment?

Member

mgol commented Oct 20, 2014

Comment author: markelog

After landing  https://github.com/jquery/jquery/pull/1662 it seems we improved here, but

Also note that using plain assignment instead of defineProperty/ies (maybe only for elements?) still makes it another 5x faster

Sounds like using defineProperties is bad idea, as of now that is, should we get back to using plain assignment?

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 20, 2014

Member

Comment author: jbedard

Even switching from defineProperties to defineProperty (since we only have one) helps a bit. But really I think something such as  https://github.com/jbedard/jquery/commit/d44885b3603dd41ebc47ac889bc55a9a0da0fd4f would be best.

Member

mgol commented Oct 20, 2014

Comment author: jbedard

Even switching from defineProperties to defineProperty (since we only have one) helps a bit. But really I think something such as  https://github.com/jbedard/jquery/commit/d44885b3603dd41ebc47ac889bc55a9a0da0fd4f would be best.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 20, 2014

Member

Comment author: gibson042

Replying to jbedard:

Even switching from defineProperties to defineProperty (since we only have one) helps a bit. But really I think something such as  https://github.com/jbedard/jquery/commit/d44885b3603dd41ebc47ac889bc55a9a0da0fd4f would be best.

I agree; our current implementation is just trying to be a bit too clever.

Member

mgol commented Oct 20, 2014

Comment author: gibson042

Replying to jbedard:

Even switching from defineProperties to defineProperty (since we only have one) helps a bit. But really I think something such as  https://github.com/jbedard/jquery/commit/d44885b3603dd41ebc47ac889bc55a9a0da0fd4f would be best.

I agree; our current implementation is just trying to be a bit too clever.

@rwaldron

This comment has been minimized.

Show comment
Hide comment
@rwaldron

rwaldron Oct 20, 2014

Member

0_o

Member

rwaldron commented Oct 20, 2014

0_o

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil Jan 30, 2015

Member

Related trac ticket moved to #1734 on github

Member

timmywil commented Jan 30, 2015

Related trac ticket moved to #1734 on github

@timmywil timmywil removed the Needs review label Jan 30, 2015

@timmywil timmywil closed this in 95fb798 Mar 4, 2015

markelog added a commit that referenced this issue Nov 10, 2015

@dmethvin dmethvin modified the milestones: 1.12/2.2, 3.0.0 Jan 7, 2016

@cssmagic cssmagic referenced this issue May 18, 2016

Open

jQuery #5

@lock lock bot locked as resolved and limited conversation to collaborators Jun 19, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.