Stop using a property bag for filter attributes

RESOLVED FIXED in Firefox 64

Status

()

enhancement
P2
normal
RESOLVED FIXED
a year ago
7 months ago

People

(Reporter: jrmuizel, Assigned: dthayer)

Tracking

(Blocks 2 bugs)

Trunk
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

(Whiteboard: [qf:p1:f64])

Attachments

(3 attachments)

We don't have time for that. (mallocs and hashtables and all that jazz)

https://perfht.ml/2z5hw0e
Whiteboard: [qf:p1][qf:f64]
Assignee: nobody → mstange
Status: NEW → ASSIGNED

Updated

11 months ago
Whiteboard: [qf:p1][qf:f64] → [qf:p1:f64]
(Assignee)

Comment 1

9 months ago
Markus, are you still looking at this? I might be able to take a look if you're swamped.
Flags: needinfo?(mstange)
I'm not working on this at the moment. I thought I had written down some notes somewhere, but I can't find them unfortunately.
Assignee: mstange → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(mstange)
(Assignee)

Updated

9 months ago
Assignee: nobody → dothayer
Status: NEW → ASSIGNED
(Assignee)

Comment 3

8 months ago
This is a more conservative optimization for bug 1417699. There's no
reason we need to be copying these everywhere, so let's just go ahead
and implement moves.
(Assignee)

Comment 4

8 months ago
This replaces the hash map of attributes with a tagged union. In
this case, all filter attributes will be stored in line, with the
exception of some more complex attributes which have an internal
nsTArray of floats. This should help avoid all the hashing and
extra heap allocations.

Depends on D4899
Comment on attachment 9006144 [details]
Bug 1417699 - Avoid copies of filter attributes when possible r=mstange

Markus Stange [:mstange] has approved the revision.
Attachment #9006144 - Flags: review+
OS: Unspecified → All
Hardware: Unspecified → All
Version: unspecified → Trunk
(Assignee)

Comment 6

7 months ago
Comment on attachment 9006145 [details]
Bug 1417699 - Replace hash map with tagged union r=mstange

(Adding the review flag back to make sure this is on your radar.)
Attachment #9006145 - Flags: review?(mstange)
It's on my radar, my review queue is just a bit backed up.
Comment on attachment 9006145 [details]
Bug 1417699 - Replace hash map with tagged union r=mstange

Markus Stange [:mstange] has approved the revision.
Attachment #9006145 - Flags: review+
Attachment #9006145 - Flags: review?(mstange)
(Assignee)

Comment 9

7 months ago
Ran into a couple of test failures because I was leaving
mAttributes empty for empty things like MergeAttributes, and
only setting mType. Since mType is now redundant though, and
since it's the only use of PrimitiveType, I figured I'd just
remove it entirely.

Depends on D4900
Comment on attachment 9010091 [details]
Bug 1417699 - Remove PrimitiveType enum r=mstange

Markus Stange [:mstange] has approved the revision.
Attachment #9010091 - Flags: review+

Comment 11

7 months ago
Pushed by dothayer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6f43cff9efd7
Avoid copies of filter attributes when possible r=mstange
https://hg.mozilla.org/integration/autoland/rev/4f3360c4f104
Replace hash map with tagged union r=mstange
https://hg.mozilla.org/integration/autoland/rev/87694c630c01
Remove PrimitiveType enum r=mstange

Comment 12

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6f43cff9efd7
https://hg.mozilla.org/mozilla-central/rev/4f3360c4f104
https://hg.mozilla.org/mozilla-central/rev/87694c630c01
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.