Opened 14 years ago

Closed 14 years ago

#7435 closed enhancement (fixed)

[patch][ccla]Optimization on console.log, non-debug

Reported by: Adam Peller Owned by: James Burke
Priority: high Milestone: 1.2
Component: Core Version: 1.1.1
Keywords: Cc: [email protected]
Blocked By: Blocking:

Description (last modified by Adam Peller)

This is arguably covered by the build change to strip console.log statements, but Julian points out that the current bootstrap implementation wraps console.log even when it's a no-op to begin with, causing performance problems when it is used frequently (also, arguably a no-no)


Attachments (2)

console-log.patch (977 bytes) - added by Adam Peller 14 years ago.
patch from Julian Cerruti (IBM, CCLA)
debug.html (1.3 KB) - added by jcerruti 14 years ago.
debug test case - goes in tests/_base

Download all attachments as: .zip

Change History (11)

Changed 14 years ago by Adam Peller

Attachment: console-log.patch added

patch from Julian Cerruti (IBM, CCLA)

comment:1 Changed 14 years ago by Adam Peller

Description: modified (diff)

comment:2 Changed 14 years ago by James Burke

I am a bit reticent to apply this patch. Seems like you get lots more performance impact by using the new console.* stripping in the build process. All console calls are really debug calls: they do not help the end user in any fashion, so still not sure this is a worthwhile optimization.

I'll think about it a little bit more. Has this patch been tested in all supported browsers, both in debug and non-debug modes?

comment:3 Changed 14 years ago by jcerruti

Just a clarification on what the problem and fix intent is:

With the code applied on r131410, even in non-debug mode you end up with a function set similar to this for all console.* methods:

console.log = function() {};

console.* = function() {  
  var a = Array.apply({}, arguments); 
  console.log(a.join(" ")); 

That means that - again, in non-debug mode - for each console.* call in the code you end up at least creating an Array, shifting the contents of the array (which is usually implemented as an element-by-element copy in the VM), a string creation and string concatenation.

For a couple of calls, this is not a big deal, but if you have a complex application with tracing capabilities, I can tell from our own experience this bogs down performance terribly. In two of our applications, this change brought the application from unusable (~60s with the browser locked up) to totally normal.

The intent of this fix is not to change any perceptible behavior on the console.* functions either in debug or non-debug mode. The only intent is to make sure that, if we're in non-debug mode - and thus console.log is indeed a stub - then make sure all the rest of the functions are also stubs and don't do Array and String manipulations unnecessarily.

I do understand that console.* stripping would take care of this problem in built releases and I look forward to that. Nevertheless, I think this addition would allow also for good performance when running in source + non-debug mode, thus still keeping debug and build aspect independent. Plus, pending pertinent testing on all platforms, I'd like to stress that the final behavior would remain unchanged, only with improved performance.

I have personally tested this only on Firefox 2.x and 3.x under Linux, but we are right now testing our applications in a wider array of browsers which I know include Firefox/Win?, IE7/Win and Safari/Mac?. I do think more extensive testing would be appropriate, but I'm afraid I don't think I would be able to provide it.

I hope this helps clarify the point on the patch and help make a decision. I would be happy to provide any additional background as needed.

Thanks for the attention to this. Julian

comment:4 in reply to:  3 Changed 14 years ago by jcerruti

Ugh, sorry. A couple of amendments:

The changeset that provoked the regression I was mentioning is r13140, and not r131410

Also, clarifying my ugly writing: our two applications went from usable in 0.4.3 to unusuable in 1.1 (due to r13140), back to usable when we applied the suggested patch attached to this ticket.

Finally, if there is a decision to apply this patch pending only testing, we would see how to find the time to do the required testing.

Regards, Julian

comment:5 Changed 14 years ago by James Burke

So I am OK applying the patch, and you made it quite small, which is great.

However, to me, this is not a critical patch, so I do not want the burden of confirming that it works. If you can confirm that it works in all supported browsers in debug and non-debug mode, then I will apply.

Thanks for taking the time to contribute!

comment:6 Changed 14 years ago by James Burke

Milestone: tbdfuture

Moving this to future, pending more testing, since we are trying to limit 1.2 tickets now to critical stoppers.

comment:7 Changed 14 years ago by jcerruti

I have just created a small test case (that I will attach shortly) and tried it on:

  • FF3/Linux
  • FF2/Linux
  • IE6/IES4Linux
  • Opera 9.52/Linux
  • IE7/WinXP
  • Safari/WinXP
  • Safari/OSX
  • FF3/OSX

And found no problem whatsoever on any of them.

Any chance of reconsidering application of this patch a bit earlier given it's low risk, high performance impact and now tests done?

Thanks, Julian

Changed 14 years ago by jcerruti

Attachment: debug.html added

debug test case - goes in tests/_base

comment:8 Changed 14 years ago by James Burke

Milestone: future1.2

Fixed in [15041]. Thanks for the patch, test and test coverage.

comment:9 Changed 14 years ago by Adam Peller

Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.