Cache DOM wrappers in the DOM object

RESOLVED FIXED in mozilla1.9.1b2

Status

()

P1
normal
RESOLVED FIXED
11 years ago
9 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)

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

11 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
(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 4

11 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
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

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

Comment 7

11 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

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

Updated

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

Comment 9

11 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

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

Updated

11 years ago
Blocks: 461563
(Assignee)

Updated

11 years ago
Blocks: 461566
(Assignee)

Updated

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

Comment 12

10 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

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

Comment 13

10 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

10 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

10 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

10 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

10 years ago
Depends on: 464114
(Assignee)

Comment 20

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

Comment 21

10 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+
Attachment #346327 - Flags: approval1.9.1b2?
Priority: P2 → P1
(Assignee)

Comment 23

10 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

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

Updated

10 years ago
Keywords: 4xp, perf

Updated

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

Updated

10 years ago
Depends on: 498897
(Assignee)

Updated

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