Closed Bug 677652 Opened 8 years ago Closed 8 years ago

Regression: one window.watch() call originates major persistent performance degradation

Categories

(Core :: JavaScript Engine, defect)

x86_64
Windows XP
defect
Not set

Tracking

()

RESOLVED WONTFIX
Tracking Status
firefox8 + wontfix

People

(Reporter: mao, Unassigned)

References

Details

(Keywords: regression)

Using window.watch() on any (even never defined) variable, and even with an empty function, causes a huge slowdown in script execution (about +80% execution time on Sunspider).

This started happening with the July 29th 2011 nightly build, and unfortunately affects all NoScript users on most pages, since a script surrogate for a popular anti-adblocker uses window.watch at the beginning of any page whose URL has the HTTP scheme.

Steps to reproduce:

1) Evaluate the following script in chrome context (e.g. in Error Console) on a recent (equal or newer than 20110729) nightly build (restart the browser)
<SNIP>
var Cc=Components.classes,Ci=Components.interfaces;
var OS = Cc['@mozilla.org/observer-service;1'].getService(Ci.nsIObserverService);
OS.addObserver({
  observe: function(doc) {
    var url = doc.documentURI;
    if(url.indexOf("/driver.html") > 0) {
      if (url.indexOf("?slow") > 0) {
        doc.defaultView.wrappedJSObject.watch("uNlIkElYvAr", function() {});
      } else {
        doc.defaultView.addEventListener("load", function() {}, false);
      }
    }
  }
},"document-element-inserted",  false);
</SNIP>
2) open http://www.webkit.org/perf/sunspider-0.9.1/sunspider-0.9.1/driver.html and take note of the final total mean time after the benchmark is done
3) open http://www.webkit.org/perf/sunspider-0.9.1/sunspider-0.9.1/driver.html?slow and take of the final total mean time after the benchmark is done
4) Compare the values taken in #2 and #3 and notice the difference. In my tests, the slowdown was more than 80% (~550ms vs ~ 300ms).

Notice that the watch() call per se appears to run actually faster than the addEventListener() call put there to balance the "fast" branch. Instead, the watch() call seems to cause a persistent performance degradation affecting the scripts executed after it in the same context.
> This started happening with the July 29th 2011 nightly build

What are the changeset IDs for the last build without the problem and the first build with the problem?

Nominating, given the noscript performance impact.

Also, chances are this is a JS engine issue.
Assignee: nobody → general
Component: DOM → JavaScript Engine
QA Contact: general → general
(In reply to Boris Zbarsky (:bz) from comment #1)
> > This started happening with the July 29th 2011 nightly build
> 
> What are the changeset IDs for the last build without the problem and the
> first build with the problem?

Last good: http://hg.mozilla.org/mozilla-central/rev/a627b24e684e
First bad: http://hg.mozilla.org/mozilla-central/rev/cef1817c3b13

> Also, chances are this is a JS engine issue.

Maybe a fallout from bug 637985 ( http://hg.mozilla.org/mozilla-central/rev/7c43296e7545 ) ?
Definitely a consequence of bug 637985.

That code was a major simplification, allowing us to delete some of the scariest code in the whole JavaScript engine. So what I'd prefer to do here is help NoScript migrate from .watch() to something faster and better.

Giorgio, what is a "script surrogate"? What "popular anti-adblocker" are we talking about here? Can you attach a copy so I can see how it works? Does the script surrogate really have to be implemented using window.watch?

If replacing this line:
>    doc.defaultView.wrappedJSObject.watch("uNlIkElYvAr", function() {});
with this:
>    Object.defineProperty(doc.defaultView.wrappedJSObject, "uNlIkElYvAr",
>                          {set: function() {}});
works, then that's what I recommend. It should eliminate the performance problem.
If that simple change *doesn't* work, it might be because watch() does some unwrapping that Object.defineProperty doesn't do. In that case, mrbkap is our man.
(In reply to Jason Orendorff [:jorendorff] from comment #3)
> So what I'd prefer to do here
> is help NoScript migrate from .watch() to something faster and better.
> 

This particular surrogate does not appear to be needed by NS any longer, but what about .watch() in general?  It may have some advantages (not actually creating the property may be valuable, perhaps others)  Is it your intent to deprecate it, in such a state it might as well be gone.
(In reply to al_9x from comment #5)
> This particular surrogate does not appear to be needed by NS any longer, but
> what about .watch() in general?

It's nonstandard and the other browsers don't implement it. I don't recommend using it.

If I could make JS 10% faster by personally throwing an egg at each .watch() user, I'd do it.

Speed with watchpoints isn't going to be a priority. We can't work on everything, and more important things are thick on the ground. (Just within the JS engine: speed without watchpoints, stability and correctness, memory usage, standard compliance, new standard-track features, developer tools.)

> It may have some advantages (not actually
> creating the property may be valuable, perhaps others)

I don't know of any advantages.

Actually, until the change in bug 637985, .watch() *did* create the property. It was observable by checks like '"uNlIkElYvAr" in window' or 'window.hasOwnProperty("uNlIkElYvAr")'. I wouldn't rule out changing the semantics back to something like that, if it made the code simpler.

> Is it your intent to deprecate it, in such a state it might as well be gone.

It's possible we could deprecate .watch() someday. If it's true that it "might as well be gone", we should remove it right away, but I think the few remaining users would rather we didn't.
(In reply to Jason Orendorff [:jorendorff] from comment #6)
> If I could make JS 10% faster by personally throwing an egg at each .watch()
> user, I'd do it.

I should make clear that I say this not out of contempt for .watch() users, but rather out of respect for their dedication and community spirit. I believe that given the choice between giving the world a 10%-faster Firefox and not having an egg on them, they would unhesitatingly choose the former.
FWIW, I've no interest in keeping watch() around for my use cases (which aren't debugging-related), because dynamic setters and Proxy are enough.

Anyway, if a certain feature causes the overall performance to *permanently* degrade by 80% (not just one slow call, but a general slowdown), I think it should be clearly documented.

So, if this is WONTFIX (I'd be fine with that), bug 637985 is "dev-doc-needed".
The change is that .watch() used to only slow down the affected property; now it affects all properties on the watched object. 

So .watch() on objects other than window is fairly harmless; window happens to be the global object, so setting a watchpoint on it affects all global variables.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.