465212 - 3.3% Tp regression on boxset [Cm2-M1.9] following checkin for bug 463073

Status: RESOLVED WONTFIX

(Camino Graveyard :: General, defect)

Description

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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.

Comment 1

[Wan-Teh Chang]

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

Comment 2

[Wan-Teh Chang]

Attachment: Diffs between NSPR_4_7_1_RTM and NSPR_4_7_3_RTM

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?

Comment 3

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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.)

Comment 4

[Wan-Teh Chang]

Attachment: Patch for prsystem.c (bug 454878)

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

Comment 5

[Samuel Sidler (old account; do not CC)]

I'll do this later tonight.

Comment 6

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(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.

Comment 7

[Boris Zbarsky [:bzbarsky]]

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?

Comment 8

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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

Comment 9

[Boris Zbarsky [:bzbarsky]]

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.

Comment 10

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Patch for nsSHistory.cpp test

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 11

[Wan-Teh Chang]

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 12

[Boris Zbarsky [:bzbarsky]]

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

Comment 13

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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.

Comment 14

[Gordon P. Hemsley [:GPHemsley]]

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]