Cache DOM wrappers in the DOM object

RESOLVED FIXED in mozilla1.9.1b2

Status

()

Core
DOM
P1
normal
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: smaug, Assigned: peterv)

Tracking

({fixed1.9.1, perf})

Trunk
mozilla1.9.1b2
fixed1.9.1, perf
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

9 years ago
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?
(Assignee)

Comment 1

9 years ago
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
(Reporter)

Comment 2

9 years ago
(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.
(Assignee)

Comment 3

9 years ago
Created attachment 341136 [details] [diff] [review]
v1
(Assignee)

Comment 4

9 years ago
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
(Reporter)

Comment 5

9 years ago
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?
(Assignee)

Comment 6

9 years ago
Don't think that's necessary (see also https://bugzilla.mozilla.org/show_bug.cgi?id=453738#c27).
(Assignee)

Comment 7

9 years ago
Created attachment 341195 [details] [diff] [review]
v1.1
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.

Updated

9 years ago
Attachment #341195 - Flags: review? → review?(mrbkap)
(Assignee)

Updated

9 years ago
Attachment #341195 - Attachment is obsolete: true
Attachment #341195 - Flags: superreview?
Attachment #341195 - Flags: review?(mrbkap)
(Assignee)

Comment 9

9 years ago
Comment on attachment 341195 [details] [diff] [review]
v1.1

Ugh, should have cleared those. I'll attach a newer version of the patch tomorrow.
(Assignee)

Comment 10

9 years ago
Created attachment 344619 [details] [diff] [review]
v2
Attachment #344619 - Flags: review?(mrbkap)
(Assignee)

Updated

9 years ago
Blocks: 461563
(Assignee)

Updated

9 years ago
Blocks: 461566
(Assignee)

Updated

9 years ago
Attachment #344619 - Flags: superreview?(jst)
Attachment #344619 - Flags: review?(mrbkap)
Attachment #344619 - Flags: review?(jst)

Updated

9 years ago
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.
(Assignee)

Comment 12

9 years ago
Created attachment 346037 [details] [diff] [review]
v2

Patch that was landed.
Attachment #344619 - Attachment is obsolete: true
Attachment #346037 - Flags: superreview+
Attachment #346037 - Flags: review+
(Assignee)

Updated

9 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Target Milestone: mozilla1.9.1 → mozilla1.9.1b2
(Assignee)

Comment 13

9 years ago
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 → ---
(Assignee)

Comment 14

9 years ago
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.
(Assignee)

Comment 18

9 years ago
Created attachment 346327 [details] [diff] [review]
v2.1

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+
(Assignee)

Comment 19

9 years ago
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.
(Assignee)

Updated

9 years ago
Depends on: 464114
(Assignee)

Comment 20

9 years ago
Patch in bug 464114 fixes some of the leaks that show up. Still hunting more.
(Assignee)

Comment 21

9 years ago
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+

Updated

9 years ago
Attachment #346327 - Flags: approval1.9.1b2?

Updated

9 years ago
Priority: P2 → P1
(Assignee)

Comment 23

9 years ago
Created attachment 347958 [details] [diff] [review]
v2.2

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?
(Assignee)

Comment 24

9 years ago
Looks like it finally sticks. Filed bug 464735 for figuring out a better fix for the leaks.
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED

Updated

9 years ago
Keywords: 4xp, perf

Updated

9 years ago
Keywords: 4xp
Keywords: fixed1.9.1
Depends on: 471395
This seems to have caused at least one change in whether chrome sees XPCNativeWrappers.  See bug 471395.
Blocks: 474456

Updated

8 years ago
Depends on: 498897
Also apparently caused bug 498897
(Assignee)

Updated

8 years ago
Duplicate of this bug: 378389
You need to log in before you can comment on or make changes to this bug.