410291 - Windows working set / private bytes regression on 12/29 (suspect bug 399587)

Status: RESOLVED WORKSFORME

(Core :: XPConnect, defect, P1)

Description

[Robert Sayre]

Visible on the tinderbox talos machines.

Looks like mrbkap is the only one in the checkin window, so assigning to him.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

huh, I don't see how either of those patches could have increased the working set so much without leaking. I'll look into it as soon as I can.

Comment 2

[Mike Schroepfer]

We should figure this out one way or the other - +ing

Comment 3

[Mike Beltzner [:beltzner, not reading bugmail]]

I noticed this when looking at the graphs to see what jemalloc had brought us, and was a little shocked to see that we haven't brought it back down.

cc'ing stuart who might have some ideas

Seems to have been caused by mrbkap's checkin of bug 399587, which was then backed out by jruderman, and then checked back in after mrbkap fixed bug 410323. When it went back in, the regression returned. It's been there ever since.

Comment 4

[Mike Beltzner [:beltzner, not reading bugmail]]

Oh, nevermind, I should have checked the dependencies, looks like the fix is in bug 410223.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Now that bug 410223 has been checked in, we should keep an eye on these numbers to see if they go back down.

Comment 6

[Olli Pettay [:smaug]]

The regression is still there, on XP talos.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

How bad was the regression here? Bkap is pretty swamped with security fixes so there's a good chance we won't get this fixed for release unless we think this is stop-ship bad. Does anyone remember a percentage?

Comment 8

[Mike Schroepfer]

Very easy to find:

http://graphs.mozilla.org/#spst=range&spss=1198954495.3714285&spse=1199029859.7485714&spstart=1196881975&spend=1203476358&bpst=Cursor&bpstart=1198954495.3714285&bpend=1199029859.7485714&m1tid=53222&m1bl=0&m1avg=0&m2tid=53258&m2bl=0&m2avg=0&m3tid=53240&m3bl=0&m3avg=0&m4tid=53280&m4bl=0&m4avg=0

Goes from ~69MB to 72MB so definitely noticeable so I wouldn't punt this.  Do we have no idea what's going on here?  

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

From Blake:

"I have one idea left on this and am trying to figure out the easiest way to implement it -- after that, I have no clue how or why the original patch could have caused (such a large) private bytes regression."

So I guess lets try that one before pushing the panic button. An alternative strategy would be to try to do it in a dot release, but it's scary to punt something without knowing how risky the fix would be.

Comment 10

[Robert Sayre]

Line 527 in XPCCrossOriginWrapper looks suspicious.

<a href="http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/js/src/xpconnect/src/XPCCrossOriginWrapper.cpp&rev=&cvsroot=/cvsroot#527">http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/js/src/xpconnect/src/XPCCrossOriginWrapper.cpp&rev=&cvsroot=/cvsroot#527</a>

I noticed that XPCWrappedNativeScope::mWrapperMap is not inspected by the cycle collector.

Comment 11

[Mike Schroepfer]

(In reply to comment #10)
> Line 527 in XPCCrossOriginWrapper looks suspicious.
> 
> <a
> href="http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/js/src/xpconnect/src/XPCCrossOriginWrapper.cpp&rev=&cvsroot=/cvsroot#527">http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/js/src/xpconnect/src/XPCCrossOriginWrapper.cpp&rev=&cvsroot=/cvsroot#527</a>
> 
> I noticed that XPCWrappedNativeScope::mWrapperMap is not inspected by the cycle
> collector.
> 

Easy to remedy?

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Sayre, if you have ideas for a patch it'd be great to have to try out. Blake is very short on time these days what with finals and such.

Comment 13

[Johnny Stenback (:jst)]

Not blocking on this bug, but we'd still love a fix...

Comment 14

[Mike Schroepfer]

(In reply to comment #13)
> Not blocking on this bug, but we'd still love a fix...
> 

Er - why?

Comment 15

[Johnny Stenback (:jst)]

Because we have more important bugs to fix. *I* (nor Jonas) wouldn't not hold the release for this. If you disagree, please plus/renom this again.

Comment 16

[Shawn Wilsher :sdwilsh]

renominating for blocking based on comment 10

Someone at least needs to look at this...

Comment 17

[Johnny Stenback (:jst)]

Not blocking the release on this, but it'd be great to know where to go here...

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

This has almost certainly been fixed in the meantime.