Make PLDHashNumber 64-bit on x86-64

RESOLVED WONTFIX

Status

()

Core
XPCOM
RESOLVED WONTFIX
10 months ago
9 months ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

unspecified
Points:
---

Firefox Tracking Flags

(firefox56 wontfix)

Details

Attachments

(1 attachment)

Comment hidden (empty)
(Assignee)

Updated

10 months ago
Assignee: nobody → ehsan
(Assignee)

Comment 1

10 months ago
Patch will come later, trying to fix some tests.
(Assignee)

Comment 2

10 months ago
Created attachment 8882776 [details] [diff] [review]
Make PLDHashNumber 64-bit on x86-64
Attachment #8882776 - Flags: review?(nfroyd)
(Assignee)

Comment 4

10 months ago
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 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.
Attachment #8882776 - Flags: review?(nfroyd) → review+

Comment 6

10 months ago
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3009a0b538da
Make PLDHashNumber 64-bit on x86-64; r=froydnj
(Assignee)

Updated

10 months ago
See Also: → bug 1377947
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
Flags: needinfo?(ehsan)

Comment 8

10 months ago
Backout by archaeopteryx@coole-files.de:
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
(Assignee)

Updated

10 months ago
Depends on: 1378015
(Assignee)

Comment 9

10 months ago
(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.
Flags: needinfo?(ehsan)

Comment 10

10 months ago
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/06f92d065a85
Make PLDHashNumber 64-bit on x86-64; r=froydnj

Comment 11

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/06f92d065a85
Status: NEW → RESOLVED
Last Resolved: 10 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
I don't understand why this is useful unless your hash table has more than 2^32 elements, which PLDHashTable cannot have.
Flags: needinfo?(ehsan)
(Assignee)

Comment 13

10 months ago
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.)
Flags: needinfo?(ehsan)
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?
Flags: needinfo?(ehsan)
(Assignee)

Comment 15

9 months ago
Yes, will do later today...
Flags: needinfo?(ehsan)
Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/fc909021a397
status-firefox56: fixed → wontfix
Target Milestone: mozilla56 → ---
You need to log in before you can comment on or make changes to this bug.