Closed Bug 457022 Opened 12 years ago Closed 12 years ago

Cache DOM wrappers in the DOM object

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla1.9.1b2

People

(Reporter: smaug, Assigned: peterv)

References

Details

(Keywords: fixed1.9.1, perf)

Attachments

(1 file, 5 obsolete files)

Currently DOM Nodes' and XHRs' wrappers are stored in their owner document.
I'm not quite that is the right way. Perhaps scriptContext or global object
would be a better place?
I've actually been working on a patch for the last week or two that stores them in the objects themselves. I need to update my tree for the recent XHR changes.
Assignee: nobody → peterv
Status: NEW → ASSIGNED
OS: Linux → All
Priority: -- → P2
Hardware: PC → All
Target Milestone: --- → mozilla1.9.1
(In reply to comment #0)
> I'm not quite that is the right way. 
+sure

Is it possible that we have several wrappers we should be able store?
Currently that isn't possible because node is the key
and wrapper the value in the hashtable.
Attached patch v1 (obsolete) — Splinter Review
We should only have one wrapper for these objects.
Patch gives a 20-30% speedup on some of the dromaeo DOM tests.
Summary: Investigate if DOM wrappers should be stored in some different way → Cache DOM wrappers in the DOM object
Comment on attachment 341136 [details] [diff] [review]
v1

>@@ -744,9 +842,19 @@ protected:
>    * NODE_* macros for the layout of the bits in this
>    * member.
>    */
>   PtrBits mFlagsOrSlots;
>+
>+  nsPreservedWrapper mWrapper;
> };
Could we store mWrapper in slots?
Don't think that's necessary (see also https://bugzilla.mozilla.org/show_bug.cgi?id=453738#c27).
Attached patch v1.1 (obsolete) — Splinter Review
Attachment #341136 - Attachment is obsolete: true
Attachment #341195 - Flags: superreview?
Attachment #341195 - Flags: review?
I wonder if it'd be worth adding a new class with no virtual methods and one member to point to the wrapper/JSObject? That way it'd be possible to QI to that class w/o doing any reference counting, and down the road we could maybe bake this in with the class we're considering adding for this pointer offsets etc as well.
Attachment #341195 - Flags: review? → review?(mrbkap)
Attachment #341195 - Attachment is obsolete: true
Attachment #341195 - Flags: superreview?
Attachment #341195 - Flags: review?(mrbkap)
Comment on attachment 341195 [details] [diff] [review]
v1.1

Ugh, should have cleared those. I'll attach a newer version of the patch tomorrow.
Attached patch v2 (obsolete) — Splinter Review
Attachment #344619 - Flags: review?(mrbkap)
Blocks: 461563
Blocks: 461566
Attachment #344619 - Flags: superreview?(jst)
Attachment #344619 - Flags: review?(mrbkap)
Attachment #344619 - Flags: review?(jst)
Attachment #344619 - Flags: superreview?(jst)
Attachment #344619 - Flags: superreview+
Attachment #344619 - Flags: review?(jst)
Attachment #344619 - Flags: review+
Comment on attachment 344619 [details] [diff] [review]
v2

XPCWrappedNative::GetUsedOnly():

+    nsWrapperCache* cache;
+    Object->QueryInterface(NS_GET_IID(nsWrapperCache), (void**)&cache);
+    if(cache &&
+       (wrapper = static_cast<XPCWrappedNative*>(cache->GetWrapper())))
+    {
+        NS_ADDREF(wrapper);
+    }
+    else
+    {
         [do expensive stuff]

If the above QI succeeds, there's no point in going into the else case even if there is no wrapper in the cache since we won't find the wrapper in the map etc either.

r+sr=jst with that.
Attached patch v2 (obsolete) — Splinter Review
Patch that was landed.
Attachment #344619 - Attachment is obsolete: true
Attachment #346037 - Flags: superreview+
Attachment #346037 - Flags: review+
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: mozilla1.9.1 → mozilla1.9.1b2
Backed out for unit test failures:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1225722086.1225726095.12145.gz&fulltext=1
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
The problem is xpcshell tests that create an XMLHttpRequest object, we'll have a null mOwner and so the PreCreate hook returns an error.
Can't we just avoid changing parentObj in that case?
And can we somehow assert (ideally statically) that the only way to get into that precreate method is with XHR?
Yeah, I think I agree with bz here, given that this case does indeed happen. It'd been nice to not have to go there, but I don't see a better way to deal with that.
Attached patch v2.1 (obsolete) — Splinter Review
Had to back out again, mailnews tinderboxes went orange. Current theory is that because their (hand-coded) QI implementations don't set result pointer to null on failure, we'll get a bogus pointer when doing

        nsWrapperCache* cache;
        CallQueryInterface(src, &cache);

I'm going to try to check in this patch (with |nsWrapperCache* cache = nsnull;|) early tomorrow morning.
Attachment #346037 - Attachment is obsolete: true
Attachment #346327 - Flags: superreview+
Attachment #346327 - Flags: review+
Had to back out again, orange due to leaks. I found one of the causes, need to release wrapper when FindTearoff fails in XPCConvert::NativeInterface2JSObject, but there's still some leaks remaining.
Depends on: 464114
Patch in bug 464114 fixes some of the leaks that show up. Still hunting more.
Actually, on trunk with the patch from bug 464114 this doesn't cause any additional leaks (borwser-chrome tests already leak on tinderbox). So this is now blocked on review and landing of the patch in bug 464114.
We really should get this in for Beta2 as has always been the plan. The bug for some reason never got the flag.
Flags: blocking1.9.1+
Attachment #346327 - Flags: approval1.9.1b2?
Priority: P2 → P1
Attached patch v2.2Splinter Review
This is the patch that I'm going to land. It fixes any remaining leaks when running mochitests (apart from the one fixed in bug 464114).
Attachment #346327 - Attachment is obsolete: true
Attachment #347958 - Flags: superreview+
Attachment #347958 - Flags: review+
Attachment #346327 - Flags: approval1.9.1b2?
Looks like it finally sticks. Filed bug 464735 for figuring out a better fix for the leaks.
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Keywords: 4xp, perf
Keywords: 4xp
Depends on: 471395
This seems to have caused at least one change in whether chrome sees XPCNativeWrappers.  See bug 471395.
Blocks: 474456
Depends on: CVE-2009-2665
Duplicate of this bug: 378389
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.