1576757 - Implement "frustrated use counters" for some DOM properties and methods

Status: NEW

(Core :: DOM: Core & HTML, enhancement, P2)

Description

[Mike Taylor [:miketaylr]]

In https://docs.google.com/spreadsheets/d/1x4zrwxmnKKtR_djCm41LE323vQRQDRBrv2cZTKtU3G8/edit#gid=127939380, there exists a prioritization of groupings of DOM members that exist in Chrome, but not Firefox. We would like to implement "frustrated use counters" for these, that is, a mechanism that increments a use counter when something we don't support is used (or attempted to be used).

Comment 1

[Mike Taylor [:miketaylr]]

[Tracking Requested - why for this release]: We'd like this work to be uplifted to 70.

Comment 2

[Tim Smith (inactive) 👨‍🔬 [:tdsmith]]

Boris is helping us work on this, so assigning; setting P1 to indicate that this is important for the current scope of bug 1479121. Moving to DOM::Core since this doesn't require work from the client telemetry team.

Comment 3

[Liz Henry (:lizzard) (relman/hg->git project)]

How is this going Mike and bz? We're heading into beta 8 of 14 so just at the halfway mark of the cycle.

Comment 4

[Mike Taylor [:miketaylr]]

I think for the DOM side of things 70 uplift is unlikely. We'll aim for 71, assuming performance characteristics allow for it.

Comment 5

[Liz Henry (:lizzard) (relman/hg->git project)]

OK, thanks. If that changes just let me know. 71 sounds more reasonable!

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Performance testcase for the HTMLDocument case

This measures how long, in ns, it takes to do a property get. Note that the cases with the fixed property names get ICed, so they're pretty fast. The long property names are much slower, because we end up jumping through the proxy machinery.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Performance measurement patch

This patch, with some variations, can be used with the testcase to get some performance numbers. The patch tests 3 different implementation strategies:

1. If the #if in missingPropUseCountersForDescriptor is set to #if 0, hash the jsid and find the use counter value, if any, that way.

2. If the #if is #if 1, do a linear search either by jsid value (this is the uncommented path in the patch right now) or by string compare (the commented path). The latter is fully functional; the former would need more machinery to actually find the right jsids to compare to.

For the linear-search cases, adjusting the for prop in instrumentedProps[0][:1] bit (which does a linear search against only a length-1 list) can be used to try different-length lists, or remove the [:1] bit to test the entire list for HTMLDocument. The full list has 21 items.

The testcase measures both search and long property names, corresponding the the third and fourth number in the testcase. Some resulting performance data in a local opt build on Linux, 5 runs for each configuration:

• Contol (pref off):

  • Short: 149 148 144 153 149
  • Long: 143 143 141 145 145

• Linear jsid compare scan

  • Short, length 1: 150 154 153 151 149
  • Long, length 1: 147 146 147 144 146
  • Short, length 4: 171 176 179 173 177
  • Long, length 4: 159 160 159 158 157
  • Short, full list: 303 304 309 306 307
  • Long, full list: 302 301 305 300 302

• Linear string-compare scan

  • Short, length 1: 145 152 150 154 150
  • Long, length 1: 147 145 147 147 144
  • Short, length 2: 170 170 170 168 167
  • Long,length 2: 154 150 155 151 152
  • Short, length 3: 180 173 172 176 174
  • Long, length 3: 164 158 157 169 161
  • Short, length 4: 182 184 189 186 183
  • Long, length 4: 171 166 170 164 165
  • Short, full list: 341 342 340 337 339
  • Long, full list: 342 341 345 339 341

• Hash lookup

  • Short: 169 166 164 163 163
  • Long: 199 196 193 197 195

So the hash lookup is, as expected, faster than linear searches once the list is long enough. Hash lookups are slower when the length of the string to hash is longer.... Even with the fairly long property names I used, the slowdown of the hash lookup is only about 30%, though from a really crappy baseline already. And again, in jitted code with literal property names the ICs would skip all this stuff and we would not have a performance hit in practice.

We could do an adaptive thing where we do a linear scan for 4-5 or fewer instrumented props and a hash lookup otherwise, but I'm not sure the complexity is worth it. So I'll try cleaning up the hashtable version and then doing a try push to see how it does on our performance tests.

Comment 8

[Boris Zbarsky [:bzbarsky]]

OK, I did a bit more poking at this and have a performance approach that works (largely suggested by jandem): switch on length then on distinguishing chars until we get to a unique possibility (if we get to anything at all), then do a single string compare. I filed 1584630 to add these to HTMLDocument and then will see about other objects.

Comment 9

[Boris Zbarsky [:bzbarsky]]

We have some use counters already landed in bug 1584630 and more on the way in bug 1588194. Do we still need to track this for 71? Seems like it might be better to track individual bugs as needed...

Comment 10

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #9)

We have some use counters already landed in bug 1584630 and more on the way in bug 1588194. Do we still need to track this for 71? Seems like it might be better to track individual bugs as needed...

Agreed! I'd like to delegate the NI to Mike as he was the one who made the original tracking request so that he's aware of this.

Comment 11

[Mike Taylor [:miketaylr]]

I'm not sure we need to track this one -- agreed with Boris. We'll nominate important bugs instead.

Comment 12

[Kagami Rosylight [:saschanaz] (they/them) (OOO until Dec 11)]

Isn't this bug 1588194? Oh, this is to use that mechanism.

Comment 13

[Kagami Rosylight [:saschanaz] (they/them) (OOO until Dec 11)]

Given we now have https://github.com/saschanaz/gecko-webidl (I need to move that to mozilla org) and https://github.com/saschanaz/bcd-idl-mapper, I think I can create a tool that analyzes MDN browser compat data and automatically add InstrumentedProps attributes for missing members.

Comment 14

[Kagami Rosylight [:saschanaz] (they/them) (OOO until Dec 11)]

(Shamelessly plugging myself to assignee unless anyone disagrees)

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Do we really want use counters for every potential API ever? That might be overkill?

Comment 16

[Kagami Rosylight [:saschanaz] (they/them) (OOO until Dec 11)]

Oh that doesn't make sense, but maybe APIs that have at least one implementation? BCD absolutely has that data.

Comment 17

[Peter Van der Beken [:peterv]]

Before we start adding these to all kinds of interfaces we need to decide whether we actually want to support this on WebIDL interfaces that don't use a proxy-based binding (essentially those not using indexed or named properties), and actually add support for that if so. Because I see us adding the annotations to various interfaces that are not using a proxy-based binding, and I don't think that's currently supported at all (and then we are just generating unused code).

Comment 18

[Kagami Rosylight [:saschanaz] (they/them) (OOO until Dec 11)]

Oops, yes, we shouldn't add no-op things! Thanks for catching that.

we need to decide whether we actually want to support this on WebIDL interfaces that don't use a proxy-based binding

I kinda assumed yes, how can we proceed further here? Do you have some counterargument, maybe it's something hard to implement?

Comment 19

[Kagami Rosylight [:saschanaz] (they/them) (OOO until Dec 11)]

I'm definitely not going to touch the bindgen for this...

Comment 20

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Not having a concrete implementation plan yet.