Devirtualize MDefinition::op()

RESOLVED FIXED in Firefox 57

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
3 months ago
3 months ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

(Blocks: 1 bug)

unspecified
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(3 attachments)

(Assignee)

Description

3 months ago
This is by far the hottest virtual function in Ion (and probably all of SpiderMonkey). Most of these calls are under def->isConstant(), def->isPhi(), etc. Even for a PGO compiler it's probably hard to optimize these calls in most cases because there are hundreds of different implementations of this function.

I added some logging and patches to devirtualize this (by storing the op in MDefinition, where we still have bits available) improve the time spent in CompileBackEnd by 4-8% on OS X with Clang so it's a pretty significant win. These calls also show up in IonBuilder so this should also improve our main thread time a bit.
See Also: → bug 1388720
(Assignee)

Comment 1

3 months ago
Created attachment 8899804 [details] [diff] [review]
Part 1 - Make MDefinition::Opcode an enum class

Better type safety and we can drop the Op_ prefix.

It's a bit more verbose - maybe we should s/Opcode/Op/ - but I don't think it matters too much.
Attachment #8899804 - Flags: review?(nicolas.b.pierron)
(Assignee)

Comment 2

3 months ago
Created attachment 8899805 [details] [diff] [review]
Part 2 - Devirtualize MDefinition::op()

This passes classOpcode to the base constructors.

Using classOpcode instead of the actual value is pretty nice because it can just be copy/pasted when adding new MIR instructions without having to update it.
Attachment #8899805 - Flags: review?(nicolas.b.pierron)
(Assignee)

Comment 3

3 months ago
Created attachment 8899806 [details] [diff] [review]
Part 3 - Make valueHash consistent

Bonus fix: MBinaryInstruction::valueHash overrides MDefinition::valueHash but it uses a different mechanism - it doesn't add the dependency and it doesn't use addU32ToHash.

This patch fixes this and asserts these derived definitions compute the same value as MDefinition::valueHash.
Attachment #8899806 - Flags: review?(nicolas.b.pierron)
Attachment #8899804 - Flags: review?(nicolas.b.pierron) → review+
Attachment #8899805 - Flags: review?(nicolas.b.pierron) → review+
Attachment #8899806 - Flags: review?(nicolas.b.pierron) → review+

Comment 4

3 months ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8eb6c914f4e0
part 1 - Make MDefinition::Opcode an enum class. r=nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e9fad014b5b
part 2 - Devirtualize MDefinition::op() by storing the op in MDefinition. r=nbp
(Assignee)

Updated

3 months ago
Keywords: leave-open

Comment 5

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8eb6c914f4e0
https://hg.mozilla.org/mozilla-central/rev/0e9fad014b5b

Comment 6

3 months ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e634da7e61a9
part 3 - Make MDefinition::valueHash() overloads more consistent. r=nbp
(Assignee)

Updated

3 months ago
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/e634da7e61a9
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.