boxset (Camino 2.0/Gecko 1.9.0/cvs head, 10.4 PPC) experienced an approximately 3.3% Tp regression immediately following the checkin for bug 463073 on Friday. 5 cycles before checkin: 527 516 520 511 517 (avg 518ms) 5 cycles after/with checkin: 536 536 537 532 535 (avg 535ms) Given that the only change was supposed to be fixing a Solaris bug, I'm not sure how this would affect Tp on Mac OS X. On the other hand, boxset has been very reliable (stable) in its Tp range.

The fix for a Solaris bug is not the only change -- the checkin was an upgrade from NSPR 4.7.1 to 4.7.3, rather than from NSPR 4.7.2 to 4.7.3. The only change in NSPR 4.7.3 is the fix for a Solaris bug, but there are many changes in NSPR 4.7.2, listed in the release notes: http://www.mozilla.org/projects/nspr/release-notes/nspr472.html After eliminating the changes that affect only non-Mac OS X platforms, the remaining suspects are a short list, essentially only two: Bug 417044 Bug 430684 (unlikely, a tiny change and affects PL_ArenaFinish only) Bug 450224 (unlikely, tiny changes) Bug 454878

I omitted the changes that cannot affect Mac OS X. Some of these remaining changes could be irrelevant to Mac OS X. I suspect the patch that changes Tp is bug 454878. Do you want me to create a special NSPR_4_7_3_RTM_EXCEPT_BUG_454878 CVS tag and try that?

Probably the best/easiest thing would be for Sam to try to back out the suspected culprit (patch for bug 454878) locally on boxset and see if that makes a difference. (We're also not sure why this regression appears not to have manifested on the Firefox 3 tree, although if there are no PPC boxen there, that might explain it.)

Sam, please try backing out this patch locally on boxset.

I'll do this later tonight.

(In reply to comment #4) > Created an attachment (id=348611) [details] > Patch for prsystem.c (bug 454878) Sam found some time and backed this patch out this afternoon. We had it in for 5 cycles (though tinderbox ate one of the cycle's numbers) on an otherwise-quiet tree. 5 cycles before NSPR 4.7.3 checkin: 527 516 520 511 517 (avg 518ms) 5 cycles after/with NSPR 4.7.3 checkin: 536 536 537 532 535 (avg 535ms) 4 cycles with backout of prsystem.c change: 514 517 520 517 (avg 516ms) (Also, the first cycle after we reverted the backout jumped back to 533ms.) That does seem to confirm that the prsystem.c change from bug 454878 is responsible for the Tp increase we observed on boxset.

So.... PR_GetPhysicalMemorySize is used to determine the sizes of several caches: 1) Memory/image (on 1.9) cache 2) sqlite history database cache 3) bfcache The old code sometimes returned the right number and sometimes 0, randomly (random based on what was on the stack, so usually reliable for a given build, but not reliable across builds or computers). If it happened to return the right number for (1) and (2) and 0 for (3) then fixing the bug could in fact lead to increased Tp by effectively re-enabling bfcache. Depending on the relationship of memory size to CPU speed, of course. As a test, could we try setting the |bytes| in nsSHistory.cpp to 0 manually and seeing whether we see exactly this 18ms win?

(In reply to comment #7) > As a test, could we try setting the |bytes| in nsSHistory.cpp to 0 manually and > seeing whether we see exactly this 18ms win? Boris, is that change something you want us to try locally on boxset, or are you wondering if that change is a valid way of testing your hypothesis?

That's a change I'd like you to try on boxset if you can. It's certainly a valid hypothesis test. ;) Just add |bytes = 0| right after the PR_GetPhysicalMemorySize call there.

Ooops, somehow I missed your comment earlier, Boris. Here's a patch for Sam to test on boxset; I assume now that NSPR_4_7_3_RTM has been packed out (and boxset has gone back to the old numbers), the expected result is that we see the regression numbers again?

Comment on attachment 350630 [details] [diff] [review] Patch for nsSHistory.cpp test Smokey: with NSPR_4_7_3_RTM backed out, we expect PR_GetPhysicalMemorySize() to fail (return 0), so this patch should not change Tp at all. In spite of that, this experiment will still verify that PR_GetPhysicalMemorySize() indeed returns 0 in NSPR_4_7_1_RTM. Since boxset doesn't do x86 <-> ppc cross compilation, it is not affected by bug 466531. So you can locally change boxset to pull NSPR_4_7_3_RTM (just modify mozilla/client.mk).

Comment 11 is correct. That patch should be a no-op on boxset right now if my analysis is correct.

Sam is on vacation for a while, so I don't know when he'll get to the backout. Just so we won't have to dig up numbers whenever he does, the NSPR (re)upgrade via bug bug 466531 moved Tp from about 520ms to about 535ms.

This bug has been buried in the graveyard and has not been updated in over 5 years. It is probably safe to assume that it will never be fixed, so resolving as WONTFIX. [Mass-change filter: graveyard-wontfix-2014-09-24]