Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Performance issue: parseCss #57

Closed
Karolusrex opened this issue Jun 6, 2018 · 6 comments
Closed

Performance issue: parseCss #57

Karolusrex opened this issue Jun 6, 2018 · 6 comments

Comments

@Karolusrex
Copy link

The parseCss method transforms a statement like translate(23px, 24px) into a transformation matrix. It constantly modifies the DOM. This is a huge performance issue, when an animation is initiated.

https://github.com/d3/d3-interpolate/blob/master/src/transform/parse.js

What do you think about parsing this with regex instead and mathematically generating the transformation matrices? Should be significantly faster. We can look for inspiration in places like https://github.com/jlmakes/rematrix for matrix generation. An simple example where performance is already a huge drain:

<html>
<head>
    <title>Learn D3 in 5 minutes</title>
</head>
<body>


<svg id="canvas"></svg>
<script src="https://d3js.org/d3.v5.js"></script>
<script>
    const circles = d3.select('svg')
        .selectAll('path')
        .data(Array(2).fill())
        .enter()
        .append('circle')
        .attr('r', 20)
    
    let xOffset = 0;
    const animate = () => {
        xOffset+=0.1
        circles.transition()
        .duration(100)
        .style('transform', (_, index) => `translate(${(index + xOffset) * 30}px, 30px)`)
        .on('end', animate);
    }
    animate();
</script>
</body>
</html>

On my brand new Macbook Pro I can see the framedrops easily with my eyes. As seen in Chrome, this triggers forced reflows. For a lot of simultaneous transitions starting at the same time, the problem is, of course, even bigger.

pastedgraphic-3

Is this a known issue? I would be interested in helping out with a pull request
@Karolusrex
Copy link
Author

Karolusrex commented Jun 6, 2018 •
edited
Loading

Turns out that there is a native API for doing this, working at least in latest Chrome, Safari, and Firefox. The window.DOMMatrix is a class for parsing these strings: https://developer.mozilla.org/en-US/docs/Web/API/DOMMatrix

According to the spec, the constructor will:

Parse transformList into parsedValue given the grammar for the CSS transform property. The result will be a , the keyword none, or failure. If parsedValue is failure, or any has values without absolute length units, or any keyword other than none is used, then return failure.

@mbostock
Copy link
Member

mbostock commented Aug 24, 2018 •
edited
Loading

Sorry for the slow response.

Unfortunately I’m not sure that DOMMatrix buys us anything: what interpolateTransform should be doing is interpolating a transform list rather than a single transform matrix; see #44. And while SVG provides a native SVGTransformList interface I don’t see that there’s a planned replacement for that will cover CSS as well.

Especially vexing is that getComputedStyle always returns the matrix(…) form for transforms, which means that there does not appear to be a way to get the current style as a transform list, and that means that it’s probably impossible to implement transform list interpolation for d3-transition, even if it’s supported in d3-interpolate.

@mbostock
Copy link
Member

Though… if we abandon our goal of interpolating transform lists and instead continue to interpolate transform matrices, then adopting DOMMatrix (or WebKitCSSMatrix) to parse seems like a win.

@mbostock
Copy link
Member

Untested branch here:

https://github.com/d3/d3-interpolate/tree/DOMMatrix

@Karolusrex
Copy link
Author

Yes, exactly like that! That is very similar to the code I'm using as well.

Like this: Karolusrex/d3-interpolate@edfdaa6

I understand the value in interpolating values instead of interpolating the matrices they represent. Perhaps interpolating the transition as matrix could be left as an option, once the feature to interpolate the values inbetween as landed. In both cases, there is no need to un-performance it by modifying the DOM.

@Fil Fil mentioned this issue Jun 4, 2020
Merged
@Fil
Copy link
Member

Fil commented Aug 23, 2020

fixed in 372a62c

@Fil Fil closed this as completed Aug 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@Fil @mbostock @Karolusrex