Warn on calling .contains() on Lists (for performance)?

[DanTup]

While profiling a script in package:win32 I noticed a large amount of time was spent in contains() on a const List. Switching it to a const Set shaved off around 10% of the entire runtime of the script:

halildurmus/win32#337

The question came up of whether the compiler/runtime could be smarter about it (eg. detect contains() on Lists and do some magic behind the scenes to make it a lookup). I'm not sure if that's feasible (although if it is, that may be better) so wondered if a lint may make sense.

I'm not certain if it would end up producing a lot of false positives (or just reports where the savings are so small that they're not worth the error to report/fix), or exactly what situations would make sense to warn, but I thought it my be worth raising for opinions at least.

[bwilkerson]

Whether it makes sense to use a set rather than a list depends on how the list is being used. For example, it isn't possible to do indexed lookup on a set, so if the index operator is being used then the conversion doesn't make sense.

We could potentially flag those cases where we can know statically whether a set is a valid replacement (probably local variables that aren't passed to any other functions), but the question is whether the lint would be useful when restricted that way. My guess is that it wouldn't be.

@srawlins and I have both thought about doing some whole-program style analyses, and this would certainly be an interesting possibility for that kind of tool.

I've also wondered several times whether performance tools like DevTools could perform this kind of analysis and recommend ways to improve performance. @kenzieschmoll

[DanTup]

the question is whether the lint would be useful when restricted that way. My guess is that it wouldn't be.

Yeah, I'd agree - in the case above they were just top level constants in another file.

@srawlins and I have both thought about doing some whole-program style analyses, and this would certainly be an interesting possibility for that kind of tool.

Yeah, that would be neat - and a better place for something like this given the other restrictions noted above.

I'll close this as I'm convinced the linter isn't the right place for it (we can re-raise it in the DevTools repo if it's the sort of thing DevTools might want to explicitly detect - I actually spotted it using DevTools CPU profiler 🙂)