1377333 - Make PLDHashNumber 64-bit on x86-64

Status: RESOLVED WONTFIX

(Core :: XPCOM, enhancement)

Comment 1

[(no longer active)]

Patch will come later, trying to fix some tests.

Comment 2

[(no longer active)]

Attachment: Make PLDHashNumber 64-bit on x86-64

Comment 3

[(no longer active)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=583ac1fcb200855fac060f29318c8eb2ec4d1680

Comment 4

[(no longer active)]

I was testing a test case which incurred expensive hashtable lookups for the CC hashtable.  It seemed like for me on my local system, with this patch applied, the hashtable lookups were hardly showing up in profiles any more.

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 8882776 [details] [diff] [review]
Make PLDHashNumber 64-bit on x86-64

Review of attachment 8882776 [details] [diff] [review]:
-----------------------------------------------------------------

Might be some minor size regressions from this, but worth taking it if we get better distributions on our hash tables.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3009a0b538da
Make PLDHashNumber 64-bit on x86-64; r=froydnj

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out on suspicion of causing frequent failures in test_general.html:

First push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=44502217ccf7cd6e6de02a47a084cbc2e33a2b01&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=111554618&repo=mozilla-inbound

13:21:15     INFO - TEST-PASS | accessible/tests/mochitest/aom/test_general.html | correct role and state on an accessible node 
13:21:15     INFO - TEST-PASS | accessible/tests/mochitest/aom/test_general.html | correct object attribute value on an accessible node 
13:21:15     INFO - TEST-PASS | accessible/tests/mochitest/aom/test_general.html | object attributes are present 
13:21:15     INFO - TEST-PASS | accessible/tests/mochitest/aom/test_general.html | correct number of attributes 
13:21:15     INFO - Buffered messages finished
13:21:15     INFO - TEST-UNEXPECTED-FAIL | accessible/tests/mochitest/aom/test_general.html | margin-left attribute is expected at 0th index - got "display", expected "margin-left"
13:21:15     INFO - SimpleTest.is@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:312:5
13:21:15     INFO - checkImplementation@chrome://mochitests/content/a11y/accessible/tests/mochitest/aom/test_general.html:100:7
13:21:15     INFO - promise callback*@chrome://mochitests/content/a11y/accessible/tests/mochitest/aom/test_general.html:15:3
13:21:15     INFO - Not taking screenshot here: see the one that was previously logged
13:21:15     INFO - TEST-UNEXPECTED-FAIL | accessible/tests/mochitest/aom/test_general.html | text-align attribute is expected at 1th index - got "explicit-name", expected "text-align"

The order of the style attributes doesn't match the expectations, and it's not just a shift: https://dxr.mozilla.org/mozilla-central/rev/6f8f10f48ace5692256efd91f011bd23054ee2ec/accessible/tests/mochitest/aom/test_general.html#101

Comment 8

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5564d2bbc09
Backed out changeset 3009a0b538da on suspicion of causing frequent failures in test_general.html. r=backout

Comment 9

[(no longer active)]

(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #7)
> The order of the style attributes doesn't match the expectations, and it's
> not just a shift:
> https://dxr.mozilla.org/mozilla-central/rev/
> 6f8f10f48ace5692256efd91f011bd23054ee2ec/accessible/tests/mochitest/aom/
> test_general.html#101

Yes, this test is essentially testing hashtable ordering.  Filed bug 1378015 with a fix.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/06f92d065a85
Make PLDHashNumber 64-bit on x86-64; r=froydnj

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/06f92d065a85

Comment 12

[Nicholas Nethercote [inactive]]

I don't understand why this is useful unless your hash table has more than 2^32 elements, which PLDHashTable cannot have.

Comment 13

[(no longer active)]

This really arose out of bug 1376563 since this type is used in hash key computations, that was the motivation...

But in the light of the journey in the past 10 days which finally brought me to bug 1379282 which made that original bug a moot issue, perhaps this was a misguided effort?  What do you think?

(I'm going to be away for a bit, so please feel free to back this out if you think this was a mistake, it should be a simple backout.  Now my current thinking is that if there are places where this type is used in hash computations and that is causing issues, perhaps we should find and fix those issues and not touch this type.)

Comment 14

[Nicholas Nethercote [inactive]]

Ehsan: I haven't backed this out myself because you've filed a number of bugs related to this topic and I wasn't sure how they all fit together and I didn't want to mess things up. But it does seem like backing this out is the right thing to do, and also that bug 1377947 and bug 1377949 can be WONTFIXed. Can you do that now that you're back?

Comment 15

[(no longer active)]

Yes, will do later today...

Comment 16

[(no longer active)]

 https://hg.mozilla.org/integration/mozilla-inbound/rev/fc909021a397a1a679726b94f2c0451c48e5beba

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/fc909021a397