Closed
Bug 1462261
Opened 6 years ago
Closed 6 years ago
Don't do PodZero/memset on HashTableEntry which is non-trivial
Categories
(Core :: JavaScript Engine, enhancement, P3)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla62
Tracking | Status | |
---|---|---|
firefox62 | --- | fixed |
People
(Reporter: Waldo, Assigned: Waldo)
Details
Attachments
(2 files)
5.61 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
5.45 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•6 years ago
|
||
AlignedStorage2 is deprecated, and it gives a false sense of security really. Better to use raw aligned sized storage. This may not strictly be *necessary* to fix this bug, but it was somewhat in the way of dealing with the warning, and this seemed a sensible time to do it.
Attachment #8976433 -
Flags: review?(jdemooij)
Assignee | ||
Comment 2•6 years ago
|
||
Attachment #8976434 -
Flags: review?(jdemooij)
Comment 3•6 years ago
|
||
Comment on attachment 8976433 [details] [diff] [review] Don't use AlignedStorage2 to implement HashTableEntry Review of attachment 8976433 [details] [diff] [review]: ----------------------------------------------------------------- WFM.
Attachment #8976433 -
Flags: review?(jdemooij) → review+
Comment 4•6 years ago
|
||
Comment on attachment 8976434 [details] [diff] [review] Abandon the idea of HashTableEntry being "POD", make its constructor explicitly initialize HashTableEntry::keyHash, give it a destructor that destroys the stored T if the entry is live, and call both constructor and destructor in the necessary places Review of attachment 8976434 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/public/HashTable.h @@ +834,2 @@ > > void destroyIfLive() { I think ~HashTableEntry is the only caller now (?), so maybe just inline destroyIfLive into that?
Attachment #8976434 -
Flags: review?(jdemooij) → review+
Updated•6 years ago
|
Priority: -- → P3
Pushed by jwalden@mit.edu: https://hg.mozilla.org/integration/mozilla-inbound/rev/233871244d80 Don't use AlignedStorage2 to implement HashTableEntry. r=jandem https://hg.mozilla.org/integration/mozilla-inbound/rev/353d2ee29591 Abandon the idea of HashTableEntry being "POD", make its constructor explicitly initialize HashTableEntry::keyHash, give it a destructor that destroys the stored T if the entry is live, and call both constructor and destructor in the necessary places. r=jandem
Comment 6•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/233871244d80 https://hg.mozilla.org/mozilla-central/rev/353d2ee29591
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Comment 7•6 years ago
|
||
Significant perf wins thanks to this bug and bug 1462540! == Change summary for alert #13332 (as of Fri, 18 May 2018 17:18:39 GMT) == Improvements: 4% JS windows7-32 pgo stylo 81,715,361.33 -> 78,556,267.33 4% JS windows7-32 opt stylo 81,634,055.34 -> 78,678,051.68 For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=13332
You need to log in
before you can comment on or make changes to this bug.
Description
•