1383125 - Bump up RECENTLY_USED_MAIN_THREAD_ATOM_CACHE_SIZE to 32

Status: RESOLVED WONTFIX

(Core :: XPCOM, defect)

Description

[(no longer active)]

Bug 1351303 added a recently used cache for atoms created during parsing class attributes on the main thread, but left the size of the cache at 31.  This means that when the compiler tries to compile <https://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/xpcom/ds/nsAtomTable.cpp#786>, in order to avoid the super expensive remainder operation, it needs to do a fair number of shifts.  Bumping this up to 32 will provide us with much more efficient code.  Compare the before to after:

Before:
  MOZ_ASSERT(NS_IsMainThread());
  nsCOMPtr<nsIAtom> retVal;
  uint32_t hash;
  AtomTableKey key(aUTF16String.Data(), aUTF16String.Length(), &hash);
  uint32_t index = hash % RECENTLY_USED_MAIN_THREAD_ATOM_CACHE_SIZE;
    6672:       89 d8                   mov    %ebx,%eax
    6674:       48 69 c0 85 10 42 08    imul   $0x8421085,%rax,%rax
    667b:       48 c1 e8 20             shr    $0x20,%rax
    667f:       89 d9                   mov    %ebx,%ecx
    6681:       29 c1                   sub    %eax,%ecx
    6683:       d1 e9                   shr    %ecx
    6685:       01 c1                   add    %eax,%ecx
    6687:       c1 e9 04                shr    $0x4,%ecx
    668a:       89 c8                   mov    %ecx,%eax
    668c:       c1 e0 05                shl    $0x5,%eax
    668f:       29 c8                   sub    %ecx,%eax
    6691:       41 89 df                mov    %ebx,%r15d
    6694:       41 29 c7                sub    %eax,%r15d
  nsIAtom* atom =
    sRecentlyUsedMainThreadAtoms[index];
    6697:       48 8d 05 00 00 00 00    lea    0x0(%rip),%rax        # 669e <NS_AtomizeMainThread(nsAString const&)+0xce>
    669e:       4e 8b 24 f8             mov    (%rax,%r15,8),%r12
  if (atom) {
    66a2:       4d 85 e4                test   %r12,%r12
    66a5:       74 21                   je     66c8 <NS_AtomizeMainThread(nsAString const&)+0xf8>
    66a7:       41 8b 54 24 08          mov    0x8(%r12),%edx
    66ac:       89 d0                   mov    %edx,%eax
    66ae:       25 ff ff ff 7f          and    $0x7fffffff,%eax

After:
  MOZ_ASSERT(NS_IsMainThread());
  nsCOMPtr<nsIAtom> retVal;
  uint32_t hash;
  AtomTableKey key(aUTF16String.Data(), aUTF16String.Length(), &hash);
  uint32_t index = hash % RECENTLY_USED_MAIN_THREAD_ATOM_CACHE_SIZE;
    6672:       41 89 df                mov    %ebx,%r15d
    6675:       41 83 e7 1f             and    $0x1f,%r15d
  nsIAtom* atom =
    sRecentlyUsedMainThreadAtoms[index];
    6679:       48 8d 05 00 00 00 00    lea    0x0(%rip),%rax        # 6680 <NS_AtomizeMainThread(nsAString const&)+0xb0>
    6680:       4e 8b 24 f8             mov    (%rax,%r15,8),%r12
  if (atom) {
    6684:       4d 85 e4                test   %r12,%r12
    6687:       74 21                   je     66aa <NS_AtomizeMainThread(nsAString const&)+0xda>
    6689:       41 8b 54 24 08          mov    0x8(%r12),%edx
    668e:       89 d0                   mov    %edx,%eax
    6690:       25 ff ff ff 7f          and    $0x7fffffff,%eax

Comment 1

[(no longer active)]

Attachment: Bump up RECENTLY_USED_MAIN_THREAD_ATOM_CACHE_SIZE to 32

Comment 2

[Eric Rahm [:erahm]]

I think generally 31 is chosen because it is prime and gets fewer collisions. So while we may reduce the costs of the index calculation, we may be bumping the collision rate which would be worse overall for performance. On the other hand it's possible primeness is less important with MFBT's hash.

Can you repeat Olli's tests from bug 1351303 to see what sort of hit rate you get?

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment #2)
> I think generally 31 is chosen because it is prime and gets fewer
> collisions. 
exactly!

Comment 4

[(no longer active)]

So initially I had tested the hit rate for general browsing and both 31 and 32 achieved around 55-58% for the sites I was testing.  But now after testing more I came across a case where there is a huge difference: Speedometer!!  There, 31 achieves ~90% hit rate, 32 achieves ~50%!  I guess I'm a new believer in 31 now.  :-)