Closed Bug 935283 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: DOM: Core & HTML, defect)

28 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28
Tracking Status
firefox27 --- unaffected
firefox28 + fixed

People

(Reporter: smaug, Assigned: peterv)

References

Details

(Keywords: perf, regression, Whiteboard: [talos_regression][qa-])

Attachments

(1 file, 1 obsolete file)

This also shows up as a regression in the Dromaeo benchmark.
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.
Attached patch v1 (obsolete) — Splinter Review
Assignee: nobody → peterv
Status: NEW → ASSIGNED
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;});
})(newDoc);
//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...
(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 on attachment 827915 [details] [diff] [review]
v1

Let's do this for now.
Attachment #827915 - Flags: review?(bzbarsky)
Comment on attachment 827915 [details] [diff] [review]
v1

r=me
Attachment #827915 - Flags: review?(bzbarsky) → review+
Attached patch v2Splinter 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]
Comment on attachment 828751 [details] [diff] [review]
v2

r=me
Attachment #828751 - Flags: review?(bzbarsky) → review+
would this bug be the reason for dromaeo DOM (query.html, modify.html) regressing around November 5th:
https://datazilla.mozilla.org/?start=1383306067&stop=1383910867&product=Firefox&repository=Mozilla-Inbound-Non-PGO&os=win&os_version=6.2.9200&test=dromaeo_dom&graph_search=c9eb4218558d&tr_id=3460240&graph=modify.html&x86=false&error_bars=false&project=talos

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:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&startdate=2013-11-05&enddate=2013-11-06
Yes, it would.
https://hg.mozilla.org/mozilla-central/rev/b539bdc3e6b7
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Flags: in-testsuite?
Whiteboard: [talos_regression] → [talos_regression][qa-]
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.