Bug 932322 regressed the testcase for Bug 916851 significantly (window.document access regression)

RESOLVED FIXED in Firefox 28



5 years ago
5 years ago


(Reporter: smaug, Assigned: peterv)


({perf, regression})

28 Branch
perf, regression
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox27 unaffected, firefox28+ fixed)


(Whiteboard: [talos_regression][qa-])


(1 attachment, 1 obsolete attachment)

This also shows up as a regression in the Dromaeo benchmark.
status-firefox27: --- → unaffected
status-firefox28: --- → affected
tracking-firefox28: --- → ?
Keywords: perf, regression
OS: Linux → All
Hardware: x86_64 → All
Version: Trunk → 28 Branch
So in the short term we may just need to put the manual expando thing back.

In the medium term, when we switch window to webidl the overhead here will drop drastically.  We won't be able to mark it [Constant], but we'll be able to mark it [Pure]... not that this would help in the cases which do method calls on the document.  :(

In the long term, I'm not sure what the right plan is.


5 years ago
Assignee: nobody → peterv

Comment 4

5 years ago
Perhaps a silly idea...
since .document is rather important to be as fast as possible, and its value doesn't change
too often, could we special case its getter.

in pseudojs something like
// unset unforgeability
(function (d) {
  window.__defineGetter__("document", function() { return d;});
//set back the unforgeability flag
Wouldn't that let jseng to optimize the getter better than in case we call to native code?

On C++ side there would be hopefully something like
WindowBinding::UpdateDocument(nsGlobalWindow* aThis, nsIDocument* aNewDoc);
>// unset unforgeability

You can't actually do that.  The whole point of unforgeability is you can't mess with the getter, and if you could unset it that would defeat the whole point...

What we _can_ maybe do if we want is to adopt an approach similar to what I've advocated for ImageDaga.data: store the relevant JSObject* or JS::Value (in this case the wrapper for the document) in the C++ object we're getting from (in this case nsGlobalWindow) and have the generated code generate an offsetof value that it stashes in the jitinfo.  Then have the JIT just read the value directly from the C++ object.  I think we'd still need to pay the ExposeObjectToActiveJS cost, and do it manually in jitcode, but that would be about it.

We should really prototype that with imagedata...

Comment 6

5 years ago
(In reply to Boris Zbarsky [:bz] from comment #5)
> >// unset unforgeability
> You can't actually do that.  The whole point of unforgeability is you can't
> mess with the getter, and if you could unset it that would defeat the whole
> point...
I was thinking some js engine magic, not anything exposed to js.
It would be exposed, since the object identity of the getter function would change with your proposal above, and you can get that function object via getOwnPropertyDescriptor().get.

Comment 8

5 years ago
Comment on attachment 827915 [details] [diff] [review]

Let's do this for now.
Attachment #827915 - Flags: review?(bzbarsky)


5 years ago
tracking-firefox28: ? → +

Comment 13

5 years ago
Created attachment 828751 [details] [diff] [review]

We need to make document non-unforgeable, so I decided to remove the WebIDL property for now. Looks pretty green on try.
Attachment #827915 - Attachment is obsolete: true
Attachment #828751 - Flags: review?(bzbarsky)
Whiteboard: [talos_regression]
would this bug be the reason for dromaeo DOM (query.html, modify.html) regressing around November 5th:

according to the graphs this is the offender: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=c9eb4218558d, yet there were 5 pushes prior that didn't build or run windows 8.

here is a link to the entire set of changes via tbpl on Nov 5th:
Last Resolved: 5 years ago
status-firefox28: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Flags: in-testsuite?
Whiteboard: [talos_regression] → [talos_regression][qa-]
You need to log in before you can comment on or make changes to this bug.