PR_GetPhysicalMemorySize returns garbage on systems with more than 2GB RAM I observed this with 32-bit x86-linux on a machine with 3GB RAM. PR_GetPhysicalMemorySize returns 18446744072724996096. The problem is with this code: long pageSize = sysconf(_SC_PAGESIZE); long pageCount = sysconf(_SC_PHYS_PAGES); LL_I2L(bytes, pageSize * pageCount); sizeof(long) is 4, so on my system "pageSize * pageCount" is a negative value. LL_I2L casts the negative value to a PRInt64, which ends up producing the bogus value. This bug ends up causing Firefox to think that it has a zero-sized memory cache on my system, which obviously degrades performance. I'll attach a candidate patch shortly.
Created attachment 196105 [details] [diff] [review] v1 patch Here's a simple patch that solves the problem. It's interesting that LL_UI2L is actually identical to LL_I2L, so it is not enough to change the macro. I need the cast to force the correct result. I decided to change the macro for clarity only.
I think it is crucial that this fix go into Firefox 1.5.
Comment on attachment 196105 [details] [diff] [review] v1 patch > #if defined(LINUX) || defined(SOLARIS) > > long pageSize = sysconf(_SC_PAGESIZE); > long pageCount = sysconf(_SC_PHYS_PAGES); >- LL_I2L(bytes, pageSize * pageCount); >+ LL_UI2L(bytes, (unsigned long) pageSize * pageCount); > > #elif defined(HPUX) I think it's better to declare pageSize and pageCount as PRUint64: PRUint64 pageSize, pageCount; LL_I2L(pageSize, sysconf(_SC_PAGESIZE)); LL_I2L(pageCount, sysconf(_SC_PHYS_PAGES)); LL_MUL(bytes, pageSize, pageCount); This is for the case of a 32-bit OS running on a system with more than 4GB RAM. Could you test this?
Yeah, I like that better. I will try it now.
Wan-Teh: Yeah, that patch works fine. However, on platforms that do not have a native 64-bit integer type, we would end up evaluating sysconf twice per LL_I2L invocation. So, temporary |long| variables are warranted.
Sigh. You can just use regular arithmetic operators to write that code. PRUint64 pageSize = sysconf(_SC_PAGESIZE); PRUint64 pageCount = sysconf(_SC_PHYS_PAGES); bytes = pageSize * pageCount; It's 2005 now. No need to use the LL_ macros.
I agree with Wan-Teh. We should not support compilers that don't offer a long long type anymore. I believe there are already several places in NSS which break those compilers, and no one has been complaining.
arg. please don't. some of us plan to deal with those edge cases.
Comment on attachment 196105 [details] [diff] [review] v1 patch We should also review the implementations for the other platforms to see if they have the same overflow problem. The code for HPUX also multiplies two 32-bit numbers (in 32-bit HP-UX), so it needs to be fixed the same way. (The type of info.physical_memory and info.page_size is _T_LONG_T, which is either int32_t or int64_t depending on whether the HP-UX OS is 32-bit or 64-bit.) The code for other platforms looks fine. In the code for AIX, we have LL_MUL(bytes, kbytes, 1024); It is wrong to use a constant in the LL_MUL macro. timeless: please name a compiler you need to support that doesn't have a 64-bit integer type (which doesn't need to be called "long long"). All the compilers NSPR support today have a 64-bit integer type. Our portability guidelines need to be reviewed from time to time, and I think it is safe to say goodbye to the LL_ macros now.
Wan-Teh: Ok, I'll take a crack at putting together a better patch shortly.
Created attachment 196269 [details] [diff] [review] v2 patch Let me know if you want a more minimal patch. I just decided to remove the LL_ macros from all #ifdef'd sections.
Comment on attachment 196269 [details] [diff] [review] v2 patch You missed this LL_ macro: > PRUint64 bytes = LL_ZERO; In the following two changes: >- LL_I2L(bytes, pageSize * pageCount); >+ bytes = (PRUint64) pageSize * pageCount; and >- LL_I2L(bytes, info.physical_memory * info.page_size); >+ bytes = (PRUint64) info.physical_memory * info.page_size; I assume the (PRUint64) applies to the first operand rather than the product (i.e., the typecast binds more tightly than the * operator), and you rely on the automatic promotion of the second operand to PRUint64. Correct?
> You missed this LL_ macro: > > > PRUint64 bytes = LL_ZERO; Yeah, that was intentional. I only changed code inside the #ifdef's, but I'll go ahead and change that as well. > I assume the (PRUint64) applies to the first operand rather > than the product (i.e., the typecast binds more tightly than > the * operator), and you rely on the automatic promotion of > the second operand to PRUint64. Correct? Yes, exactly. If you prefer, I will make the casting more explicit.
Created attachment 196273 [details] [diff] [review] v2.1 patch BTW, I tested this patch on Linux-x86 (3 GB RAM), OSX-ppc (1 GB RAM), and WinXP (1 GB RAM).
Comment on attachment 196273 [details] [diff] [review] v2.1 patch I think we should fix this for Firefox 1.5
fixed on NSPR trunk and NSPRPUB_PRE_4_2_CLIENT_BRANCH. I assume that we will want to also land this on the following branches: MOZILLA_1_8_BRANCH and NSPR_4_6_BRANCH. I will await explicit approval before doing so.
Yes, MOZILLA_1_8_BRANCH and NSPR_4_6_BRANCH should be kept in sync for as long as possible.