Closed
Bug 1355494
Opened 8 years ago
Closed 7 years ago
Consider using eslint to report synchronous layout flushes
Categories
(Developer Infrastructure :: Lint and Formatting, enhancement, P4)
Tracking
(Not tracked)
RESOLVED
WONTFIX
People
(Reporter: florian, Unassigned)
Details
(Whiteboard: [fxperf])
Calls to getComputedStyle or getBoundingRect dramatically affect performance as they force a synchronous layout flush.
I wonder if we should make eslint reject new calls to these methods, unless a specific annotation has been placed on the line.
Comment 1•8 years ago
|
||
Tests that count layout flushes during performance-critical operations seem like a better idea to me.
It often comes down to being smarter about when to flush layout and cache the results rather than completely eliminating the call. There are perfectly valid reasons for using getComputedStyle and friends, and imho people shouldn't have to learn some meta language to annotate their valid code. (We have started this anti-pattern with global-scope imports, but last I heard this was going to be temporary.)
Updated•8 years ago
|
No longer blocks: photon-performance-triage
Updated•8 years ago
|
Priority: -- → P2
Whiteboard: [photon-performance]
Updated•7 years ago
|
Priority: P2 → P3
Whiteboard: [photon-performance] → [reserve-photon-performance]
Updated•7 years ago
|
Priority: P3 → P4
Updated•7 years ago
|
Product: Testing → Firefox Build System
Updated•7 years ago
|
Whiteboard: [reserve-photon-performance] → [fxperf]
Comment 2•7 years ago
|
||
Given that we now have the reflow tests for common operations, and it is relatively straightforward to add new ones, I am tempted to suggest we should wontfix this. I think Dão's reasoning in comment #1 is correct. Florian?
Flags: needinfo?(florian)
Reporter | ||
Comment 3•7 years ago
|
||
I agree with wontfix because I don't see this going anywhere, I don't think eslint is going to be the right tool here.
Reflow tests only cover the operations we know we care about. The reason why I suggested eslint here is that it would be nice to have something giving a heads up at review time (or even earlier if possible) that some code is going to perform poorly.
I don't fully agree with comment 1, in that I don't think we have valid reasons left to trigger sync flushes in new code. We mostly have bugs we should fix, and some of them take a large amount of effort compared to the expected benefit so I don't seem everything getting fixed soon.
Detecting with eslint when code using getComputedStyle is valid or not would be hard, and I agree that putting annotations everywhere would be a pain. At the time I filed this, I was still expecting that most getBoundingRect calls could be easily changed to getBoundsWithoutFlushing, and that would have been easy to detect with eslint. Now we are moving in a direction where getBoundingRect is OK, but the code should wait for a window.promiseDocumentFlushed callback. This isn't trivial to detect with eslint.
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: needinfo?(florian)
Resolution: --- → WONTFIX
Updated•6 years ago
|
Version: Version 3 → 3 Branch
Updated•2 years ago
|
Product: Firefox Build System → Developer Infrastructure
You need to log in
before you can comment on or make changes to this bug.
Description
•