Performance Regression: Styled components - continually adding `style` blocks to `head`

[bryphe]

Since using master, I've noticed gradual slowdowns as I've been using Oni. I traced it down to to styles being continually added at the head:

It seems that, whenever the props change, a new ID is generated and added to the head. We should look at ways to limit this - performance is critical.

[bryphe]

There's multiple issues here:

• As the addition of styles occurs, it causes layout to get more expensive, as well as essentially being a memory leak.
• The addition of styles also causes much larger repaint areas than needed.

We can't afford to compromise on performance - for properties that are changing frequently, it seems like it's best to use inline styles, as not only does that prevent the head from blowing up, it keeps allows layout and paint to be scoped to the element (important for things like cursor movemement... for example, #1129 is now much worse - instead of just the row being repainted, the whole <ActiveWindow /> needs to be repainted!)

[bryphe]

@samvv @Akin909 - FYI - given the performance issues I'm seeing here, I think it's fine to use styled-components for things where we would use CSS (like static values, or even theming, animations/transitions), but for dynamically changing values, its best for performance to continue to use inline styles.

Let me know if you have feedback or other ideas on how to handle this!

[bryphe]

For now I'll revert the dynamic setting of styles in the high-frequency code paths (like Cursor, CursorLine, etc)

[akinsho]

@bryphe I've been using styled components for a while and I haven't encountered this problem, not immediately sure what could be causing it, a quirk with electron or perhaps our implementation so far not sure if were referencing both the typed version of styledComponents in common as well as the untyped styled-components lib though I've not known that to be an issue, I can look at some SC repos's issues and also look at the implementation so far, seems there are issues with commas instead of the semi-colons in places which could lead to invalid css that SC may be trying to reproduce each time.

EDIT: Seems fair to remove it from wherever it's causing a slow down

[bryphe]

Hmm, seems like it is our implementation - for static styles, styled-components is smart enough to hash it and then re-use the style based on the hash.

Unfortunately though, for styles that change, like this code: https://github.com/onivim/oni/blob/9fa83a62ff35f535cef81d5e5f2d8a3f418df01d/browser/src/UI/components/Cursor.tsx#L64

const CursorContainer = withProps<ICursorRendererProps>(styled.div)`
    visibility: ${props => props.visible ? "visible" : "hidden"};
    position: absolute;
    left: ${props => props.x}px;
    top: ${props => props.y}px;
    width: ${props => isInsertCursor(props) ? "0" : props.width}px;
    height: ${props => props.height || "0"}px;
    line-height: ${props => props.height}px;
    color: ${props => props.textColor};
    font-family: ${props => props.fontFamily};
    font-size: ${props => props.fontSize};
    opacity: ${props => props.isLoaded ? 1 : 0};
    transition: opacity 0.35s ease 0.25s;
    /* Cover up 'holes' due to subpixel rendering on canvas */
    padding-left: 1px;
    padding-right: 1px;
    margin-left: -1px;

Every time one of those props changes, like the x or y (which change very frequently!), lots of stuff happens, since there is no hash match and styled-components has to regenerate the style:

• The style block is re-parsed and then auto-prefixed
• A new class name is created, and the style/class is appended to the head

This is a perf killer because it causes a large repaint, and blows up the styles - since every time x or y changes, a new style is created (and all that parsing has to happen).

[bryphe]

Because of that, IMO, the best bet in order to preserve perf is:

• Use styled-components for static things, shared classes, or things that depend on CSS functionality (animations, media queries, etc)
• Use inline styles for properties that change frequently

[akinsho]

That's sounds like a good plan, tbh seeing that component like that is a bit of an ah ha moment as I've definitely never written or seen a styled component that would have that many props alternating 100s+ times in a very short space of time

[akinsho]

@bryphe having a look at the styled components repo I've found this issue relating to exactly this problem, it is discussed thoroughly here. The conclusion is the same as yours aka inline style or a styled components escape hatch that is equivalent to that. The example the issue includes is

const HighlyDynamicComponent = styled.div.attrs({
  style: props => ({
    background: props.bg,
  })
})`
  color: blue;

UPDATE: heres an example for one of our components

const Marker = styled.div.attrs({
    style: (props: MarkerProps) => ({
        top: `${props.line / props.bufferSize * props.height}px`,
    }),
})`
    position: absolute;
    height: 2px;
    background-color: ${(props: { color: string }) => props.color};
    width: 100%;
    `

Docs: https://www.styled-components.com/docs/api

[bryphe]

Cool, nice find @Akin909 ! 👍

That looks like exactly what we need. I started going back to inline styles for a few of the components in #1148, but I'll see if I can leverage this for some of the remaining ones, and we can try it out moving forward!

[bryphe]

Adding a test to help provide a "safety net" here in #1150

[bryphe]

While investigating this... saw our OSX tests were 'silently failing'. Looking into this with #1153 - our safety net was really broken!

[bryphe]

This is addressed now, along with the 'silently failing' tests - our test suite is in better state now with #1153 , and we now at least have some guard on performance regressions (by watching the added elements, and ensuring the paint rectangle is at least some lower bound).