Ruby on Rails | Screencasts | Download | Documentation | Weblog | Community | Source

Ticket #4477 (closed defect: fixed)

Opened 3 years ago

Last modified 3 years ago

Element.extend cripples $() performance

Reported by: newmanb@stanford.edu Assigned to: sam
Priority: low Milestone: 1.x
Component: Prototype Version: 1.1.0 RC1
Severity: normal Keywords: Element.extend $() caching
Cc: jonathan@bluewire.net.nz, t.fuchs@wollzelle.com, white@nwtpro.com

Description

Since the $() shorthand is used throughout the Prototype library and shows up in functions that are likely to be found inside critical inner loops (e.g. the Element methods), it seems unfortunate that Element.extend() is now an unavoidable part of $().

As anecdotal evidence, I have a bit of code that filters a list of messages while the user types keywords into a searchbox. On the first pass, Element.extend() applies itself to every element being filtered and thus multiplies the runing time by a factor of 8 (relative to a version where Element.extend() is not called in $()). That's a critical threshold for the average list size, since the time jumps from about half a second (acceptable) to more than four seconds (makes me sad). Snappiness is lost.

Could we possibly leave $() alone? The invocation of Element.extend needs to be opt-in rather than no-way-out. I might be persuaded otherwise if someone would point me to a thread where the merits of Element.extend are discussed in detail. For the time being, though, my local version of $() will have to live apart from the svn trunk.

Regards, Ben

Attachments

prototype-perf-patch.diff (1.0 kB) - added by Thomas Fuchs <t.fuchs@wollzelle.com> on 03/30/06 17:31:37.
Patch to make $ faster on Firefox and Safari
prototype-perf-patch-i2.diff (1.5 kB) - added by Thomas Fuchs <t.fuchs@wollzelle.com> on 03/30/06 18:03:37.
Ahem, better working version
prototype-perf-patch-i3.diff (1.5 kB) - added by Thomas Fuchs <t.fuchs@wollzelle.com> on 03/30/06 18:53:16.
Fixes problems on Safari
prototype-perf-patch-i4.diff (1.2 kB) - added by Thomas Fuchs <t.fuchs@wollzelle.com> on 03/30/06 19:00:29.
DRYing up the code

Change History

03/29/06 07:50:53 changed by devslashnull@gmail.com

I'm not sure if #4465 addresses the same concern, but I thought I should mention it. I share your concern Ben, and yet am excited by the possibilities of Element.extend. Would you mind creating a page that demonstrates this, making sure to use the version after [4094]?

03/29/06 15:57:22 changed by sam

Element.extend is here to stay, so let's focus on ways we can improve its performance.

The caching in [4094] should improve performance and memory usage by an order of magnitude, since we now create a constant number of function objects instead of scaling linearly with the number of DOM elements. I'm open to further discussion of optimizations, so please do post suggestions in this ticket. Thanks.

03/29/06 16:03:30 changed by sam

FWIW, if you need performance and don't want the benefits of Element.extend, you can put this on your pages after including prototype.js: 

<script type="text/javascript">Element.extend = Prototype.K</script>

03/29/06 21:02:07 changed by anonymous

But this would break scriptaculous effects, as they use Element.extend, right?

03/29/06 23:34:17 changed by newmanb@stanford.edu

  • priority changed from high to low.
  • severity changed from major to normal.

Anonymous is quite right. Doing exactly what Sam says, though very easy, would be a little drastic :] The idea is still good, though: I'll just set Element.extend equal to the identity function (prototype.k) temporarily while I do things that have no need of its benefits, and then reset it. That will solve my problem.

If I manage to do anything to speed up Element.extend, I'll post about it here.

03/30/06 07:40:32 changed by jonathan@bluewire.net.nz

  • cc set to jonathan@bluewire.net.nz.

03/30/06 17:31:37 changed by Thomas Fuchs <t.fuchs@wollzelle.com>

  • attachment prototype-perf-patch.diff added.

Patch to make $ faster on Firefox and Safari

03/30/06 17:34:50 changed by madrobby

Added proof-of-concept patch that does the extension directly on the HTMLElement.prototype and includes support for Safari and Firefox. It falls back to the current implementation on browsers that don't support HTMLElement.

Note that this patch will break script.aculo.us 1.6, as it doesn't include support for making additionally added methods on Element.Methods available (the addition of this would be trivial).

03/30/06 18:03:37 changed by Thomas Fuchs <t.fuchs@wollzelle.com>

  • attachment prototype-perf-patch-i2.diff added.

Ahem, better working version

03/30/06 18:04:06 changed by madrobby

(note that some things are broken in Safari with this, investigating...)

03/30/06 18:53:16 changed by Thomas Fuchs <t.fuchs@wollzelle.com>

  • attachment prototype-perf-patch-i3.diff added.

Fixes problems on Safari

03/30/06 19:00:29 changed by Thomas Fuchs <t.fuchs@wollzelle.com>

  • attachment prototype-perf-patch-i4.diff added.

DRYing up the code

03/30/06 19:59:39 changed by madrobby

  • cc changed from jonathan@bluewire.net.nz to jonathan@bluewire.net.nz, t.fuchs@wollzelle.com.

03/31/06 12:47:35 changed by Tommy <tommy.skaue@gmail.com>

fyi

I experienced an abnormal timedelay on IE after updating to 1.5.0_pre1, but after applying the fix by Thomas (4094) everything was back to normal. So for me the performance is satisfying (again).

04/01/06 03:59:20 changed by anonymous

  • cc changed from jonathan@bluewire.net.nz, t.fuchs@wollzelle.com to jonathan@bluewire.net.nz, t.fuchs@wollzelle.com, white@nwtpro.com.

04/02/06 09:57:32 changed by madrobby

Please note that patch prototype-perf-patch-i4.diff has been applied to edge rails.

04/06/06 04:52:04 changed by sam

  • status changed from new to closed.
  • resolution set to fixed.

(In [4181]) prototype: *1.5.0_rc0* (April 5, 2006)

* Modify HTMLElement.prototype and short-circuit Element.extend where possible. Closes #4477. [Thomas Fuchs]

* Only observe window.onunload in IE for Mozilla bfcache support. Closes #3726. [Mike A. Owens]

* Don't redefine Array.prototype.shift. Closes #4138. [leeo]

* Prevent redefining Array.prototype.reverse more than once. Closes #3951. [brettdgibson@gmail.com]

* Fix Enumerable.min/Enumerable.max to recognize the value 0. Closes #3847, #4190. [rubyonrails@brainsick.com, Martin Bialasinski]

* Change Ajax.getTransport to prefer XMLHttpRequest in anticipation of IE 7. Closes #3688. [jschrab@malicstower.org, sam]

* Make Array.prototype.flatten more robust. Closes #3453. [Martin Bialasinski, sam]

* Fix evalScripts from crashing Firefox if script includes 'var'. Closes #3288, #4165. [rahul@ntag.com, sam]

* Scope iterators locally. Closes #4589. [sam]

04/06/06 10:48:50 changed by rubyonrails@brainsick.com

Someone just went through and cleaned up the iterators scope. This introduces another global iterator variable (property).

We're repeating mistakes.

04/06/06 11:26:25 changed by newmanb@stanford.edu

Well, not my mistake, but it's pretty trivial. Did you fix this yourself? Reminding folks about the problem is the important thing. Anyways: 

Index: src/dom.js
===================================================================
--- src/dom.js	(revision 4184)
+++ src/dom.js	(working copy)
@@ -29,7 +29,7 @@
   
   if (!element._extended && element.tagName && element != window) {
     var methods = Element.Methods, cache = Element.extend.cache;
-    for (property in methods) {
+    for (var property in methods) {
       var value = methods[property];
       if (typeof value == 'function')
         element[property] = cache.findOrStore(value);
@@ -251,7 +251,7 @@
   
   if(typeof HTMLElement != 'undefined') {
     var methods = Element.Methods, cache = Element.extend.cache;
-    for (property in methods) {
+    for (var property in methods) {
       var value = methods[property];
       if (typeof value == 'function')
         HTMLElement.prototype[property] = cache.findOrStore(value);

Ben